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.

Sign up for GitHub

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

Jump to bottom

Allow node wallet to receive payjoin transactions #301

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Allow node wallet to receive payjoin transactions #301

wants to merge 7 commits into from

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented May 28, 2024 •
edited
Loading

fully addresses #177
blocked by lightningdevkit/rust-lightning#3024

@jbesraa jbesraa force-pushed the pj-receiver branch 5 times, most recently from 22b2e82 to ff71f08 Compare May 29, 2024 13:58
@jbesraa
Copy link
Contributor Author

jbesraa commented May 30, 2024

Currently blocked by the failing CI https://github.com/lightningdevkit/ldk-node/actions/runs/9287478277/job/25556603253?pr=301, a tracking issue on rust bitcoin is already open here rust-bitcoin/rust-bitcoinconsensus#32.
@tnull I wonder if this is a deal breaker or we can introduce the bitcoinconsensus feature optionally for non-uniffi which mean the payjoin receiver wouldnt be available for bindings

@tnull
Copy link
Collaborator

tnull commented May 30, 2024 •
edited
Loading

@tnull I wonder if this is a deal breaker or we can introduce the bitcoinconsensus feature optionally for non-uniffi which mean the payjoin receiver wouldnt be available for bindings

Hmm, unfortunately I think it's a bit of a deal breaker as we really want to keep the bindings and Rust as congruent as possible. Especially so as, in contrast to 'receive-and-open-channel', payjoin send/receive would be expected to be a client-side feature that mobile users might benefit from?

I mean if it really blocks your progress we could consider landing the code behind a cfg(payjoin) flag so it's already in the codebase and can be used, e.g., in tests. But I'd be hesitant to fully expose it in Rust and not expose it in the bindings at all.

Two potential ways to go about it:

a) Push for a fix in bitcoinconsensus.
b) Manually implement the necessary consensus checks, if they are not too many?

@jbesraa
Copy link
Contributor Author

jbesraa commented Jun 3, 2024

@tnull I wonder if this is a deal breaker or we can introduce the bitcoinconsensus feature optionally for non-uniffi which mean the payjoin receiver wouldnt be available for bindings

Hmm, unfortunately I think it's a bit of a deal breaker as we really want to keep the bindings and Rust as congruent as possible. Especially so as, in contrast to 'receive-and-open-channel', payjoin send/receive would be expected to be a client-side feature that mobile users might benefit from?

I mean if it really blocks your progress we could consider landing the code behind a cfg(payjoin) flag so it's already in the codebase and can be used, e.g., in tests. But I'd be hesitant to fully expose it in Rust and not expose it in the bindings at all.

Two potential ways to go about it:

a) Push for a fix in bitcoinconsensus. b) Manually implement the necessary consensus checks, if they are not too many?

yup, working on option a. Got in touch with BitcoinZavior to get some input from him about bindings, otherwise I am researching/experimenting with solutions.

Now that lightningdevkit/rust-lightning#3024 is ready for reivew, this bitcoinconsensus binding issue is the only blocker for both regular payjoin receiver and receive-open-channel flows.

@jbesraa jbesraa force-pushed the pj-receiver branch 5 times, most recently from 035ad65 to 1eac75e Compare June 7, 2024 11:15
@jbesraa
Copy link
Contributor Author

jbesraa commented Jun 7, 2024

@tnull Apparently I dont need the bitcoinconsensus feature after all.

Previously I was checking if the transaction sent by the sender is broadcast-able, but the payjoin protocol requires that only if payjoin used in a non-interactive receiver, as mentioned here https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-original-psbt-checklist. So the binding issue is longer blocking this.

@tnull
Copy link
Collaborator

tnull commented Jun 7, 2024

@tnull Apparently I dont need the bitcoinconsensus feature after all.

Previously I was checking if the transaction sent by the sender is broadcast-able, but the payjoin protocol requires that only if payjoin used in a non-interactive receiver, as mentioned here https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-original-psbt-checklist. So the binding issue is longer blocking this.

Cool, let me know when this is ready for review.

What is the idea for testing this? Could we add some integration tests testing sending/receiving? To this end, since both PRs only hold a single commit and this one is based on #295, would it make sense to close #295, and add a third commit adding integration tests?

@jbesraa
Copy link
Contributor Author

jbesraa commented Jun 7, 2024

integration test already added here https://github.com/lightningdevkit/ldk-node/pull/301/files#diff-ebba69f49eedcd3dd6a656eff964c6184b6d29c02fbad6a49d3b41c87cc3de79

I am very close to open the third PR (payjoin->channel opening), and based on the work I have done there I expect a couple of minor changes here. Will let you know when its RFR.

@jbesraa jbesraa force-pushed the pj-receiver branch 11 times, most recently from b1ac224 to d137df1 Compare June 11, 2024 09:38
@jbesraa jbesraa mentioned this pull request Jun 11, 2024
Closed
@jbesraa jbesraa marked this pull request as ready for review June 11, 2024 10:07
@jbesraa
Copy link
Contributor Author

jbesraa commented Jun 11, 2024 •
edited
Loading

@tnull I think this is ready for review.
This adds the payjoin receiver part and also opening channels from payjoin request (and the sender commit is CP here).

There are a couple of things that needs discussion, I added a comment about them in the code:
https://github.com/lightningdevkit/ldk-node/pull/301/files#diff-4db9c4cca2ff80c2ad80cbf7111959e148b31ee24cb354f8620c417047fe7b46R132

https://github.com/lightningdevkit/ldk-node/pull/301/files#diff-03a28135b3f471a98f1c40eb8a4dcc7c4759f4bf9aabc83c12c71f14d68e00c1R518

let me know if this PR is small enough for review and I can close #295.

@jbesraa jbesraa force-pushed the pj-receiver branch 2 times, most recently from 763e710 to f2652f9 Compare June 11, 2024 12:15
@tnull
Copy link
Collaborator

tnull commented Jun 12, 2024 •
edited
Loading

@tnull I think this is ready for review. This adds the payjoin receiver part and also opening channels from payjoin request (and the sender commit is CP here).

There are a couple of things that needs discussion, I added a comment about them in the code: https://github.com/lightningdevkit/ldk-node/pull/301/files#diff-4db9c4cca2ff80c2ad80cbf7111959e148b31ee24cb354f8620c417047fe7b46R132

https://github.com/lightningdevkit/ldk-node/pull/301/files#diff-03a28135b3f471a98f1c40eb8a4dcc7c4759f4bf9aabc83c12c71f14d68e00c1R518

Ah, unfortunately these links don't resolve for me by now. Could you describe the issues here.

let me know if this PR is small enough for review and I can close #295.

Hmm, you're right, ~2k lines changes is a mouth full, so let start with #295, land it behind a cfg-gate, and then remove the cfg flag here once we're positive it's ready and tested, i.e., also after the FundingSigned PR landed and we upgraded to the corresponding LDK release.

@tnull tnull mentioned this pull request Jun 12, 2024
Open
@jbesraa jbesraa force-pushed the pj-receiver branch 9 times, most recently from 40e9a9d to 801aaf5 Compare June 17, 2024 14:46
@jbesraa jbesraa force-pushed the pj-receiver branch 3 times, most recently from af1c223 to e5f3a6b Compare June 28, 2024 08:00
@jbesraa jbesraa force-pushed the pj-receiver branch 2 times, most recently from c5a0f3c to 6209d96 Compare July 10, 2024 15:47
jbesraa added 5 commits July 10, 2024 18:50
@jbesraa
Allow to send payjoin transactions
ced7777 
Implements the payjoin sender as describe in BIP77.

This would allow the on chain wallet linked to LDK node to send payjoin
transactions.
@jbesraa
Allow to send payjoin transactions
a8efc7f 
Implements the payjoin sender as describe in BIP77.

This would allow the on chain wallet linked to LDK node to send payjoin
transactions.
@jbesraa
Add ability to receive Payjoin tx
ef8e586 
Implements the payjoin receiver part as describe in BIP77.

This would allow the on chain wallet linked to LDK node to receive payjoin
transactions.

Receiving a payjoin transaction requires first to enroll with the
configured Payjoin directory and listening to our enrolled subdirectory
for upcoming request. When a request received, we validate it as
specified in BIP78, prepare our Payjoin proposal and send it back to the
payjoin sender via the subdirectory.
@jbesraa
Open LN channel from incoming Payjoin tx
bd8074f 
This commit allows users to schedule a channel that will opened
once a Payjoin request received. This can save users 1 extra onchain
transaction fees.

The Payjoin flow is normal with the following caveats:
1. We use `Payjoin::ProvisionalProposal::substitue_output_address` to
point to the multisig output script as retrived from
`LdkEvent::FundingGeneratingReady`.
2. We dont try to preserve privacy in Payjoin channel opening
transactions.
3. We wait with our response to the Payjoin sender until a
`Ldk::Event::FundingTxBroadcastSafe` event is received.
@jbesraa
Add Payjoin example
8721105 
..to run the example `cargo run --example ldk-node-with-payjoin-support`
@tnull
Copy link
Collaborator

tnull commented Oct 16, 2024

@jbesraa We now upgraded LDK to 0.0.124/0.0.125 which shipped your 'unsafe funding' work, which unblocks this PR I think?

Do you still intend to finish these Payjoin PRs? Has there been any update on the rust-payjoin side w.r.t to the identifiers etc. we previously discussed?

@jbesraa
Copy link
Contributor Author

jbesraa commented Oct 16, 2024

@jbesraa We now upgraded LDK to 0.0.124/0.0.125 which shipped your 'unsafe funding' work, which unblocks this PR I think?

Do you still intend to finish these Payjoin PRs? Has there been any update on the rust-payjoin side w.r.t to the identifiers etc. we previously discussed?

Yup, I do.

I see the rust-payjoin issue is still open payjoin/rust-payjoin#336. How do you suggest approaching this? should we think about internal solution? or try to submit a pr to rust-payjoin ?

@tnull
Copy link
Collaborator

tnull commented Oct 29, 2024

Yup, I do.

Cool!

I see the rust-payjoin issue is still open payjoin/rust-payjoin#336. How do you suggest approaching this? should we think about internal solution? or try to submit a pr to rust-payjoin ?

I think we previously discussed doing the latter, i.e., that first rust-payjoin needs to expose some identifiers, etc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbesraa @tnull