feat: add Payjoin v2 send online command with fallback to v1 #200

mehmetefeumit · 2025-06-04T04:54:25Z

Description

This change implements the online command for sending a Payjoin. In the context of Payjoin, sending refers to creating the original PSBT which can be used as the vanilla transaction, sending it to the receiver for their input's addition, and broadcasting the Payjoin proposal which the receiver sends back.

The communication between the sender and the receiver can be done synchronously (v1) or asynchronously (v2) through a OHTTP relay as the middle man. This implementation supports both: if the OHTTP relay is not defined or there is an issue with the v2 process, it will use/fallback to v1. Otherwise, it will use v2.

Persistence

Note that this change does not implement persistence. If a sending session is started and is left during the process (i.e. when the sender is waiting for the receiver to contribute their UTXOs and send back the Payjoin proposal), then the user cannot pick up from where they left of.

I made the decision myself due to the UX complexity of adding the resume functionality. bdk-cli only has two layers of commands. For example, you can implement bitcoin-cli wallet send_payjoin, but you cannot do bitcoin-cli wallet send_payjoin resume. So there is a UX/interface decision to be made on how the resume would be implemented.

We could do send_payjoin, retrieve_payjoin, and resume_send_payjoin, etc., but that is also a hit on the complexity of the interface and the UX of Payjoin in general. Same with adding another flag for resuming, but the more I thought about it, the more complex it got...

So I left it as Noop persistence for now, but do let me know if you have ideas!

Notes to the reviewers

Tested on my local regtest with rpc, both v1 and v2, using the OHTTP relays and directories in the payjoin-cli README. Feel free to do the same, and let me know if there are comments, documentation, etc. which should be changed with this. I was not able to find any...

Here's a sample command you can use to test on regtest:

cargo run --features=rpc -- -n regtest wallet -a USERNAME:PASSWORD -w WALLET_NAME -e DESCRIPTOR send_payjoin --ohttp_relay RELAY1 --ohttp_relay RELAY2 --fee_rate 1 --uri URI

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

[] I've added tests for the new feature <--- There is no testing suite for this.
I've added docs for the new feature
[N/A] I've updated CHANGELOG.md

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

src/handlers.rs

spacebear21

At a high level, V2 Payjoin should not fallback to V1. We should be able to tell whether the receiver is a v1 or v2 receiver based on the pj param in the BIP21 uri and use the appropriate version. If v2 send cannot happen due to ohttp_relay not being provided or some other issue, it should simply abort and print an error message. Generally bdk-cli should reproduce the behavior of the payjoin-cli reference implementation.

Persistence

Note that this change does not implement persistence.

I made the decision myself due to the UX complexity of adding the resume functionality. bdk-cli only has two layers of commands. For example, you can implement bitcoin-cli wallet send_payjoin, but you cannot do bitcoin-cli wallet send_payjoin resume. So there is a UX/interface decision to be made on how the resume would be implemented.

Here too we can look at payjoin-cli for guidance. There are three commands: send, receive and resume. The resume command resumes all pending send sessions and receive sessions in parallel.

That being said, it would be fine for this PR to only implement send functionality and add receive/resume as follow-ups. Especially as payjoin persistence is undergoing a major overhaul with the Session Event Log, persistence can be left as a NoOp for now.

src/handlers.rs

spacebear21 · 2025-06-04T17:01:49Z

src/commands.rs

+        fee_rate: u64,
+        /// URL of the OHTTP relay.
+        #[arg(env = "PAYJOIN_OHTTP_RELAY", long = "ohttp_relay")]
+        ohttp_relay: Option<String>,


payjoin-cli uses a list of ohttp relays so that it can fallback to another relay in case one is not working, it may be useful to follow the same pattern for bdk-cli instead of an Option.

spacebear21 · 2025-06-04T17:35:07Z

src/utils.rs

+#[cfg(any(
+    feature = "electrum",
+    feature = "esplora",
+    feature = "cbf",
+    feature = "rpc"
+))]


This feature gate seems unnecessary for this function

spacebear21 · 2025-06-04T17:52:12Z

src/handlers.rs

+            // We need to re-introduce the UTXO information of the original PSBT's inputs
+            // back into the Payjoin proposal we received from the receiver. The receiver strips
+            // the transaction of the information, so we need to add the `..._utxo` information back
+            // before we can re-sign our input(s) into the transaction.
+            let mut original_inputs_iter = input_pairs(&mut original_psbt).peekable();
+            for (proposal_txin, proposal_psbt_input) in input_pairs(&mut psbt) {
+                if let Some((orig_txin, orig_psbt_input)) = original_inputs_iter.peek() {
+                    if proposal_txin.previous_output == orig_txin.previous_output {
+                        proposal_psbt_input.witness_utxo = orig_psbt_input.witness_utxo.clone();
+                        proposal_psbt_input.non_witness_utxo =
+                            orig_psbt_input.non_witness_utxo.clone();
+                        original_inputs_iter.next();
+                    }
+                }
+            }


I don't think it's true that the receiver strips the utxo information? See here.

The only thing that gets stripped are the sender signatures afaict.

I'm not sure that a receiver necessarily strips it but I'm not sure you can depend on them not being stripped. Since BDK needs the info to find the keys associated with a script, you might want to add it back in as is done here.

coveralls · 2025-06-07T05:15:57Z

Pull Request Test Coverage Report for Build 15434070095

Details

0 of 231 (0.0%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 2.291%

Changes Missing Coverage	Changed/Added Lines	%
src/commands.rs	2	0.0%
src/utils.rs	7	0.0%
src/handlers.rs	222	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	1	3.04%

Totals
Change from base Build 15422078682:	-0.4%
Covered Lines:	25
Relevant Lines:	1091

💛 - Coveralls

mehmetefeumit · 2025-06-11T18:08:41Z

@spacebear21 Made all the changes regarding multiple OHTTP relays, no fallback, and commit organization. I was not able to find a built-in method for determining whether a URI is for Payjoin v1 or v2, so just looking for a '#' at the moment to differentiate, which is hacky.

I'll probably create an issue and work on that function if PDK does not have a way to tell the version of a URI at the moment.

DanGould · 2025-06-14T21:48:34Z

Cargo.toml

@@ -21,6 +21,8 @@ log = "0.4"
 serde_json = "1.0"
 thiserror = "2.0.11"
 tokio = { version = "1", features = ["full"] }
+payjoin = { version = "0.23.0", features = ["v1", "v2", "io"] }
+minreq = { version = "2.13.2", features = ["https"] }


If you're using the io feature, you're already depending on reqwest, so minreq is a second HTTP client. If you want to use minreq to reduce the dependency burden I recommend also writing the payjoin/io feature functions using minreq.

DanGould

I suppose this might work, but have specific concerns about how v1/v2 URIs are distinguished, which you brought up. I am going to propose a fix upstream to make this aspect more ergonomic, but for now, there are certainly improvements in the abstraction presented here, specifically separating the ohttp-relay selection logic from the payjoin protocol logic.

src/handlers.rs

src/utils.rs

src/handlers.rs

Cargo.toml

Before introducing the payjoin sending logic (which will also make use of broadcasting), moving the broadcast logic out of the current Broadcast command since the same logic will be shared between both Broadcast and the SendPayjoin which is implemented later.

mehmetefeumit · 2025-06-24T21:48:56Z

@DanGould Made the changes in the comments. Major changes are (1) checking and determining a healthy OHTTP relay before we go ahead with everything else, (2) no failover from v2 to v1 based on any random failure, and instead using the v2 extraction to determine whether we should do one or the other.

Also added some additional error messaging for the user to understand why the cli may have failed over to v1 (no relays, URI being v1, etc.). Thanks for the catches and the recommendations.

mehmetefeumit · 2025-06-24T21:52:18Z

src/handlers.rs

+                async {
+                    for relay in ohttp_relays {
+                        if let Ok(_) =
+                            payjoin::io::fetch_ohttp_keys(relay.clone(), req_ctx.endpoint()).await


This is similar to what payjoin-cli is doing here. The main difference is that we are not really using the keys here since we are not storing them anywhere. Currently, the entire implementation is stateless -- there is no resume functionality, and we do not store information on the OHTTP relays either. So we're just using the fetch... function here to determine if a OHTTP relay passed in the arguments is healthy. And then scrap the keys, and leave everything to the extract_v2 call.

`{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

github-project-automation bot added this to BDK-CLI Jun 4, 2025

mehmetefeumit force-pushed the payjoin-send branch from 2d54a78 to 6932c53 Compare June 4, 2025 05:04

mehmetefeumit mentioned this pull request Jun 4, 2025

wip(payjoin-send): failing to sign the original PSBT #191

Closed

9 tasks

mehmetefeumit commented Jun 4, 2025

View reviewed changes

src/handlers.rs Show resolved Hide resolved

spacebear21 suggested changes Jun 4, 2025

View reviewed changes

mehmetefeumit force-pushed the payjoin-send branch 2 times, most recently from 89481cb to c02b679 Compare June 11, 2025 17:28

mehmetefeumit requested a review from spacebear21 June 11, 2025 18:08

DanGould reviewed Jun 14, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

mehmetefeumit force-pushed the payjoin-send branch from c02b679 to d8f821d Compare June 24, 2025 21:44

mehmetefeumit commented Jun 24, 2025

View reviewed changes

feat: add Payjoin send for both v1 and v2

dd6a582

mehmetefeumit force-pushed the payjoin-send branch from d8f821d to dd6a582 Compare June 24, 2025 21:53

DanGould mentioned this pull request Jul 16, 2025

Abstract common sender functionality, not heirarchy payjoin/rust-payjoin#884

Merged

feat: add Payjoin v2 send online command with fallback to v1 #200

Are you sure you want to change the base?

feat: add Payjoin v2 send online command with fallback to v1 #200

Conversation

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

Uh oh!

Description

Persistence

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacebear21 Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 7, 2025

Pull Request Test Coverage Report for Build 15434070095

Details

💛 - Coveralls

Uh oh!

mehmetefeumit commented Jun 11, 2025

Uh oh!

DanGould Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

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!

Uh oh!

Uh oh!

mehmetefeumit commented Jun 24, 2025

Uh oh!

mehmetefeumit Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mehmetefeumit commented Jun 4, 2025 •

edited

Loading