[RFC] Implement a way to do BOLT 12 Proof of Payment #3593

vincenzopalazzo · 2025-02-07T23:14:43Z

This is already possible if we manually_handle_bolt12_invoices, but
this approach requires a significant amount of work from the user.

This commit adds the bolt12 invoice to the PaymentSend event when
the payment is completed. This always gives the user the option to
provide proof of payment.

I am opening this up for an initial review, but I need to look inside the DNSSECProof to add this proof, but
probably be also a follow-up PR

~~Opening in draft for now to make the CI happy~~

Fixes: #3344

lightning/src/events/mod.rs

codecov · 2025-02-10T20:28:47Z

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.71%. Comparing base (18166d0) to head (8f5dc5e).
Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/offers_tests.rs	85.71%	1 Missing and 1 partial ⚠️
lightning/src/ln/outbound_payment.rs	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3593      +/-   ##
==========================================
+ Coverage   88.51%   89.71%   +1.19%     
==========================================
  Files         149      149              
  Lines      114968   126473   +11505     
  Branches   114968   126473   +11505     
==========================================
+ Hits       101768   113467   +11699     
+ Misses      10707    10609      -98     
+ Partials     2493     2397      -96

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

Thank you for picking this up! Sorry about the delay in review, not sure why this slipped through the cracks.

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2025-02-13T22:18:46Z

lightning/src/ln/outbound_payment.rs

@@ -2291,6 +2305,7 @@ impl OutboundPayments {
 					payment_metadata: None, // only used for retries, and we'll never retry on startup
 					keysend_preimage: None, // only used for retries, and we'll never retry on startup
 					invoice_request: None, // only used for retries, and we'll never retry on startup
+					bolt12_invoice: None, // only used for retries, and we'll never retry on startup! TODO(vincenzopalazzo): double check this


Hmm, yea, we actually might want to store these somehow. Can happen in a later PR though.

Ok so I will add a comment inside the main issue to track it down and I will open a followup PR in the next week! as I was planning for the DNS sec proof. Thanks

lightning/src/events/mod.rs

vincenzopalazzo · 2025-02-16T13:06:50Z

Thanks for the review @TheBlueMatt, I applied the suggestion in the review. In the meanwhile, I rebased on top of main to fix the conflict and ran the CI again

lightning/src/ln/outbound_payment.rs

lightning/src/offers/mod.rs

TheBlueMatt · 2025-03-05T16:02:37Z

lightning/src/ln/outbound_payment.rs

@@ -1090,6 +1112,7 @@ impl OutboundPayments {
 							.take()
 							.ok_or(Bolt12PaymentError::UnexpectedInvoice)?
 							.invoice_request,
+						static_invoice: String::from("we should have the static invoice here"),


Yea, should be easy to pass it form the callsite.

yep sorry I pushed the code but I was not happy for the approach, I am rolling back the static invoice work and see if I can come up with a clean approach today!

Thanks for the comments, tho!

vincenzopalazzo · 2025-03-05T19:40:43Z

Ok @TheBlueMatt this PR is really funny to work! I worked to clean up the PR a bit and divide the code to support both Bolt12 and static invoices. However, I am currently working on supporting the async payment functionality. As far as I understand, the code involving async payments is a bit tricky to support for the following reasons (code available 85c3f73).

The async workflow is initialized with initiate_async_payment, where we pass down the payment_id and the static invoice value. Then we call static_invoice_received, which generates the event PendingOutboundPayment::StaticInvoiceReceived. After that, we iterate over the entry in send_payment_for_static_invoice and follow the Bolt12 pay code path (at this point, we need to pass down the static invoice).

The only way I have to access the static invoice when passing it down inside PendingOutboundPayment::StaticInvoiceReceived breaks the storage code because the static invoice is not available without cfg(async_payment).

Is there any way to use the persistence macros (impl_writeable_tlv_based_enum_upgradable) with a cfg?
If we want to read and pass the static invoice in PendingOutboundPayment::StaticInvoiceReceived, we could store a serialized version of the static invoice (a sequence of bytes), but this seems rather hacky.
I still need to delve into the mechanics of async payment as implemented in LDK, but if I understand correctly, there is a refactoring that I can do to make access to the static invoice a bit easier?

I would prefer the 3rd point, but this requires a follow-up PR to rework a little bit how the async payment is handled and I do not know if there is any plan for that,t or there is other code that will be depending on this architecture in the pipeline.

Probably @valentinewallace will be interested in this conversation

TheBlueMatt · 2025-03-10T14:28:57Z

Is there any way to use the persistence macros (impl_writeable_tlv_based_enum_upgradable) with a cfg?

Generally we just copy and paste the impl with different cfg flags and go back and clean it up after we drop the cfg.

I'll let @valentinewallace comment on the rest.

valentinewallace · 2025-03-10T16:00:42Z

Ok @TheBlueMatt this PR is really funny to work! I worked to clean up the PR a bit and divide the code to support both Bolt12 and static invoices. However, I am currently working on supporting the async payment functionality. As far as I understand, the code involving async payments is a bit tricky to support for the following reasons (code available 85c3f73).

The async workflow is initialized with initiate_async_payment, where we pass down the payment_id and the static invoice value. Then we call static_invoice_received, which generates the event PendingOutboundPayment::StaticInvoiceReceived. After that, we iterate over the entry in send_payment_for_static_invoice and follow the Bolt12 pay code path (at this point, we need to pass down the static invoice).

The only way I have to access the static invoice when passing it down inside PendingOutboundPayment::StaticInvoiceReceived breaks the storage code because the static invoice is not available without cfg(async_payment).
* Is there any way to use the persistence macros (`impl_writeable_tlv_based_enum_upgradable`) with a cfg?

* If we want to read and pass the static invoice in `PendingOutboundPayment::StaticInvoiceReceived`, we could store a serialized version of the static invoice (a sequence of bytes), but this seems rather hacky.

* I still need to delve into the mechanics of async payment as implemented in LDK, but if I understand correctly, there is a refactoring that I can do to make access to the static invoice a bit easier?
I would prefer the 3rd point, but this requires a follow-up PR to rework a little bit how the async payment is handled and I do not know if there is any plan for that,t or there is other code that will be depending on this architecture in the pipeline.

Probably @valentinewallace will be interested in this conversation

Hey sorry I missed this tag. In #3618 right now we actually un-cfg-gate StaticInvoices. Would that solve this? I could try to get conceptual ACKs on that to unblock you if so.

vincenzopalazzo · 2025-03-11T20:52:22Z

Generally we just copy and paste the impl with different cfg flags and go back and clean it up after we drop the cfg.

Yes, but in this case, we need to duplicate all the impl_writeable_tlv_based_enum_upgradable code which sounds a really bad thing, I think I will go to steal the commit 890ce7d and see how the code will looks like :) but I think StaticInvoice out of the cfg is all what I needs

We need to include static invoices in the public API as part of the onion messages we're adding for static invoice server support. Utilities to create these static invoices and other parts of the async receive API will remain cfg-gated for now. Generally, we can't totally avoid exposing half baked async receive support in the public API because OnionMessenger is parameterized by an async payments message handler, which can't be cfg-gated easily.

This commit prepares the code to pass down the BOLT12 invoice inside the `PaymentSent` event. To achieve this, the `bolt12` field has been added to the `PendingOutboundPayment::Retryable` enum, allowing it to be attached to the `PaymentSent` event when the payment is completed. Link: lightningdevkit#3344 Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

To enable proof of payment, we need to share the bolt12 invoice with the library user. This is already possible if we `manually_handle_bolt12_invoices`, but this approach requires a significant amount of work from the user. This commit adds the bolt12 invoice to the PaymentSend event when the payment is completed. This allows the user to always have the option to perform proof of payment. Link: lightningdevkit#3344 Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

…Received Inject the static invoice inside the PendingOutboundPayment::StaticInvoiceReceived to allow to use the static invoice inside the PendingOutboundPayment::PaymentReceived. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

vincenzopalazzo · 2025-03-12T15:10:52Z

lightning/src/offers/static_invoice.rs

@@ -379,6 +379,7 @@ impl StaticInvoice {
 		self.signature
 	}

+	#[allow(unused)] // TODO: remove this once we remove the `async_payments` cfg flag


looks like that cherry-picking does this, so will fix when the Ci will report all the failure!

vincenzopalazzo

I’ve pushed the first clean version of this PR. This should accomplish what we’re looking for with minimal changes to the static invoice workflow.

It still needs some work to pass the CI completely, but the issues appear to be unrelated to the PR itself and require further investigation. I’m going to grab some coffee while waiting for the CI to finish. In the meantime, this should be ready for review if anyone has the time.

vincenzopalazzo marked this pull request as draft February 7, 2025 23:18

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from c7e9e61 to cc714ae Compare February 7, 2025 23:33

vincenzopalazzo commented Feb 7, 2025

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from a5c211c to 9cdbb44 Compare February 10, 2025 16:59

vincenzopalazzo marked this pull request as ready for review February 10, 2025 16:59

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 9cdbb44 to 8f5dc5e Compare February 10, 2025 20:19

TheBlueMatt reviewed Feb 13, 2025

View reviewed changes

vincenzopalazzo mentioned this pull request Feb 16, 2025

BOLT 12 Proof of Payment #3344

Open

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 7da68f6 to 0e7b503 Compare February 16, 2025 13:05

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 0e7b503 to a4897fb Compare February 16, 2025 13:11

vincenzopalazzo requested a review from TheBlueMatt February 16, 2025 13:12

TheBlueMatt reviewed Feb 18, 2025

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from a4897fb to 2c65d6a Compare March 3, 2025 19:15

TheBlueMatt reviewed Mar 5, 2025

View reviewed changes

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 9b72246 to 2c65d6a Compare March 5, 2025 16:31

vincenzopalazzo mentioned this pull request Mar 7, 2025

Multiple InvoiceReceived events for single pay_for_offer call #3653

Open

TheBlueMatt requested a review from valentinewallace March 10, 2025 14:27

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch 3 times, most recently from 4364dc2 to 9d64585 Compare March 11, 2025 21:15

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 9d64585 to 87aec7e Compare March 12, 2025 11:06

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 4f9d1ad to 48ebbde Compare March 12, 2025 14:39

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 48ebbde to 734c955 Compare March 12, 2025 14:49

vincenzopalazzo added 2 commits March 12, 2025 15:59

vincenzopalazzo force-pushed the macros/proof-of-payment-bolt12 branch from 734c955 to 525c01e Compare March 12, 2025 15:01

vincenzopalazzo commented Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Implement a way to do BOLT 12 Proof of Payment #3593

[RFC] Implement a way to do BOLT 12 Proof of Payment #3593

vincenzopalazzo commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Feb 13, 2025

vincenzopalazzo Feb 16, 2025

vincenzopalazzo commented Feb 16, 2025 •

edited

Loading

TheBlueMatt Mar 5, 2025

vincenzopalazzo Mar 5, 2025

vincenzopalazzo commented Mar 5, 2025

TheBlueMatt commented Mar 10, 2025

valentinewallace commented Mar 10, 2025

vincenzopalazzo commented Mar 11, 2025

vincenzopalazzo Mar 12, 2025

vincenzopalazzo left a comment

[RFC] Implement a way to do BOLT 12 Proof of Payment #3593

Are you sure you want to change the base?

[RFC] Implement a way to do BOLT 12 Proof of Payment #3593

Conversation

vincenzopalazzo commented Feb 7, 2025 • edited Loading

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Feb 13, 2025

Choose a reason for hiding this comment

vincenzopalazzo Feb 16, 2025

Choose a reason for hiding this comment

vincenzopalazzo commented Feb 16, 2025 • edited Loading

TheBlueMatt Mar 5, 2025

Choose a reason for hiding this comment

vincenzopalazzo Mar 5, 2025

Choose a reason for hiding this comment

vincenzopalazzo commented Mar 5, 2025

TheBlueMatt commented Mar 10, 2025

valentinewallace commented Mar 10, 2025

vincenzopalazzo commented Mar 11, 2025

vincenzopalazzo Mar 12, 2025

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

vincenzopalazzo commented Feb 16, 2025 •

edited

Loading