[RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute #3719

vincenzopalazzo · 2025-04-08T20:21:49Z

Matt noted during the last round of review the following:

Oof. Sorry I missed this until now. This is not, in fact, "only used for retries", we use it on claims only, in fact. If a user is relying on the event field for PoP, what this can mean is that we can initiate a send, restart with a stale ChannelManager, notice the payment is pending, then when it claims fail to provide the invoice (only the preimage) to the payer.
In practice, to fix this, we'll need to include the PaidBolt12Invoice in the HTLCSource::OutboundRoute, I believe.

This commit is trying to store the PaidBolt12Invoice inside the HTLCSource::OutboundRoute, but this is not enough because we have to store the invoice also inside the PendingOutboundPayment.

Fixes: #3714

ldk-reviews-bot · 2025-04-08T20:21:51Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Yea, this all makes sense I think. Sorry to make you undo some of the previous patch.

lightning/src/ln/channelmanager.rs

ldk-reviews-bot · 2025-04-14T19:32:09Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

vincenzopalazzo · 2025-04-22T17:55:07Z

Thanks Matt! I rebased on the main to fix the commit checks and addressed some of the review comments. We should be good for another round

ldk-reviews-bot · 2025-04-22T17:55:37Z

✅ Added second reviewer: @wpaulino

TheBlueMatt

Conceptually looks good, but there's still quite a few FIXMEs left, do you want to address those?

lightning/src/ln/channelmanager.rs

ldk-reviews-bot · 2025-04-24T17:56:30Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

vincenzopalazzo · 2025-04-25T20:02:47Z

Jumping in finishing this tomorrow, sorry for the delay!

codecov · 2025-04-26T12:58:05Z

Codecov Report

Attention: Patch coverage is 67.56757% with 12 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (c6921fa) to head (b9a07da).
Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	66.66%	3 Missing and 1 partial ⚠️
lightning/src/offers/signer.rs	0.00%	4 Missing ⚠️
lightning/src/offers/static_invoice.rs	0.00%	3 Missing ⚠️
lightning/src/ln/channelmanager.rs	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3719      +/-   ##
==========================================
- Coverage   89.15%   89.11%   -0.05%     
==========================================
  Files         156      157       +1     
  Lines      123837   124017     +180     
  Branches   123837   124017     +180     
==========================================
+ Hits       110408   110515     +107     
- Misses      10754    10812      +58     
- Partials     2675     2690      +15

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot · 2025-04-28T00:00:11Z

🔔 2nd Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-04-28T12:48:30Z

🔔 1st Reminder

Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Good to squash. Also would prefer a more concise commit message as opposed to just quoting a message from Matt.

wpaulino · 2025-04-28T17:33:29Z

lightning/src/ln/channelmanager.rs

 				read_tlv_fields!(reader, {
 					(0, session_priv, required),
 					(1, payment_id, option),
 					(2, first_hop_htlc_msat, required),
 					(4, path_hops, required_vec),
 					(5, payment_params, (option: ReadableArgs, 0)),
 					(6, blinded_tail, option),
+					(7, bolt12_invoice, option),


Should this be even to guarantee that users will receive the invoice in the PaymentSent event?

This is a good point, and I do not remember if this was discussed with some other review in the past with @TheBlueMat, but I am fine with the idea to make it mandatory and set to None id the value is not specified.

I was missing the case that you described, the reason that It is not event is for backwards compatibility but I guess None guarantee the same things

It moves the PaidBolt12Invoice (BOLT 12 invoice) into HTLCSource::OutboundRoute to ensure the invoice is available for proof-of-payment and event emission, as discussed in issue lightningdevkit#3714. The commit also updates hashing implementations and derives to ensure correct behavior when the invoice is present, and propagates the invoice through relevant payment and event structures. This fixes a potential issue where the invoice could be lost on restart, affecting PoP reliability. Link: lightningdevkit#3714 Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

TheBlueMatt

A few nits.

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2025-04-29T14:10:47Z

lightning/src/offers/signer.rs

@@ -263,6 +263,13 @@ pub(super) struct MetadataMaterial {
 	encrypted_payment_id: Option<[u8; PaymentId::LENGTH]>,
 }

+impl core::hash::Hash for MetadataMaterial {


You can drop this (sadly I think we'd need to find a way to hash the hmac if we kept it, which would be annoying) if...

TheBlueMatt · 2025-04-29T14:10:48Z

lightning/src/offers/signer.rs

@@ -57,7 +57,7 @@ const ASYNC_PAYMENTS_HELD_HTLC_HMAC_INPUT: &[u8; 16] = &[9; 16];

 /// Message metadata which possibly is derived from [`MetadataMaterial`] such that it can be
 /// verified.
-#[derive(Clone)]
+#[derive(Clone, Hash)]


You can drop this if...

TheBlueMatt · 2025-04-29T14:10:57Z

lightning/src/offers/static_invoice.rs

 /// The contents of a [`StaticInvoice`] for responding to an [`Offer`].
 ///
 /// [`Offer`]: crate::offers::offer::Offer
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Hash)]


You drop this.

vincenzopalazzo · 2025-04-29T16:34:02Z

Mh! ok I am trying to go back to this @TheBlueMatt #3719 (comment)

Do you think that having the invoice will bring any benefit for the has function inside the OutboundRoute ? There is any case where the hash will be equal, where the invoice will change the hash instead?

In addition, we would like to keep the odd value in here or changing this to even?

TheBlueMatt · 2025-04-29T17:35:25Z

I think its best to just not because it violates the Rust API guidelines etc. We shouldn't need to hash the Invoice fields tho, because we can just impl hash to hash the bytes which are the full encoded invoice.

vincenzopalazzo changed the title ~~move the bolt12 invoice inside HTLCSource::OutboundRoute~~ [RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute Apr 8, 2025

vincenzopalazzo marked this pull request as ready for review April 14, 2025 17:16

ldk-reviews-bot requested a review from wpaulino April 14, 2025 17:17

wpaulino requested review from TheBlueMatt and removed request for wpaulino April 14, 2025 18:07

TheBlueMatt reviewed Apr 14, 2025

View reviewed changes

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

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

vincenzopalazzo force-pushed the macros/pop-with-persistance branch 3 times, most recently from 637983d to 5c25caf Compare April 22, 2025 17:54

vincenzopalazzo requested a review from TheBlueMatt April 22, 2025 17:55

ldk-reviews-bot requested a review from wpaulino April 22, 2025 17:55

TheBlueMatt reviewed Apr 24, 2025

View reviewed changes

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

vincenzopalazzo force-pushed the macros/pop-with-persistance branch from 94cadd5 to 19ba769 Compare April 26, 2025 12:32

vincenzopalazzo requested a review from TheBlueMatt April 26, 2025 12:48

vincenzopalazzo force-pushed the macros/pop-with-persistance branch from 19ba769 to 6af45f9 Compare April 26, 2025 12:49

vincenzopalazzo force-pushed the macros/pop-with-persistance branch from 6af45f9 to b7ea0de Compare April 26, 2025 15:21

wpaulino reviewed Apr 28, 2025

View reviewed changes

vincenzopalazzo force-pushed the macros/pop-with-persistance branch from b7ea0de to b9a07da Compare April 29, 2025 10:34

TheBlueMatt approved these changes Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute #3719

[RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute #3719

vincenzopalazzo commented Apr 8, 2025

ldk-reviews-bot commented Apr 8, 2025 •

edited

Loading

TheBlueMatt left a comment

ldk-reviews-bot commented Apr 14, 2025

vincenzopalazzo commented Apr 22, 2025

ldk-reviews-bot commented Apr 22, 2025

TheBlueMatt left a comment

ldk-reviews-bot commented Apr 24, 2025

vincenzopalazzo commented Apr 25, 2025

codecov bot commented Apr 26, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 28, 2025

ldk-reviews-bot commented Apr 28, 2025

wpaulino left a comment

wpaulino Apr 28, 2025

vincenzopalazzo Apr 28, 2025

TheBlueMatt left a comment

TheBlueMatt Apr 29, 2025

TheBlueMatt Apr 29, 2025

TheBlueMatt Apr 29, 2025

vincenzopalazzo commented Apr 29, 2025

TheBlueMatt commented Apr 29, 2025

[RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute #3719

Are you sure you want to change the base?

[RFC] move the bolt12 invoice inside HTLCSource::OutboundRoute #3719

Conversation

vincenzopalazzo commented Apr 8, 2025

ldk-reviews-bot commented Apr 8, 2025 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 14, 2025

vincenzopalazzo commented Apr 22, 2025

ldk-reviews-bot commented Apr 22, 2025

TheBlueMatt left a comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 24, 2025

vincenzopalazzo commented Apr 25, 2025

codecov bot commented Apr 26, 2025 • edited Loading

Codecov Report

ldk-reviews-bot commented Apr 28, 2025

ldk-reviews-bot commented Apr 28, 2025

wpaulino left a comment

Choose a reason for hiding this comment

wpaulino Apr 28, 2025

Choose a reason for hiding this comment

vincenzopalazzo Apr 28, 2025

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Apr 29, 2025

Choose a reason for hiding this comment

TheBlueMatt Apr 29, 2025

Choose a reason for hiding this comment

TheBlueMatt Apr 29, 2025

Choose a reason for hiding this comment

vincenzopalazzo commented Apr 29, 2025

TheBlueMatt commented Apr 29, 2025

ldk-reviews-bot commented Apr 8, 2025 •

edited

Loading

codecov bot commented Apr 26, 2025 •

edited

Loading