Include routing failures in `Bolt12PaymentError` #3171

jkczyz · 2024-07-11T23:11:05Z

Instead of returning Ok when path finding fails, allow returning a RetryableSendFailure from send_payment_for_bolt12_invoice. The BOLT11 and BOLT12 outbound payment initiation code differ in that the latter re-uses the retry path (i.e., find_route_and_send_payment). The drawback of this is that Ok is returned even if there is an error finding a route. Refactor send_payment_for_bolt12_invoice such that it re-uses find_initial_route instead so that errors can be returned.

Fixes #3159

Instead of returning Ok when path finding fails, allow returning a RetryableSendFailure from send_payment_for_bolt12_invoice. Follow up commits will return such failures.

This will be used later in send_payment_for_bolt12_invoice instead of find_route_and_send_payment as it will allow for returning RetryableSendFailure when path finding fails.

jkczyz · 2024-07-11T23:22:04Z

I attempted to remove PendingOutboundPayment::InvoiceReceived, but it would result in producing an Event::InvoiceRequestFailed instead of a Event::PaymentFailed in some cases. The latter needs a payment hash, which we have in the invoice but isn't easily accessible by OutboundPayments::abandon_payment.

codecov-commenter · 2024-07-11T23:27:10Z

Codecov Report

Attention: Patch coverage is 82.35294% with 15 lines in your changes missing coverage. Please review.

Project coverage is 91.07%. Comparing base (6035c83) to head (b697fbe).
Report is 72 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/outbound_payment.rs	82.14%	13 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3171      +/-   ##
==========================================
+ Coverage   89.74%   91.07%   +1.32%     
==========================================
  Files         121      121              
  Lines       99858   111148   +11290     
  Branches    99858   111148   +11290     
==========================================
+ Hits        89622   101230   +11608     
+ Misses       7561     7427     -134     
+ Partials     2675     2491     -184

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

tnull

Mostly looks good to me, just a few nits/questions.

It also seems this only addresses the first point of #3159, i.e., we currently don't early-abort before sending the invoice request if we're positive that we'll be unable to send the payment. Should we address this in another PR? In this case it might be worth opening a separate issue for it?

tnull · 2024-07-12T09:56:04Z

lightning/src/ln/outbound_payment.rs

+			Ok(route) => route,
+			Err(e) => {
+				let reason = match e {
+					RetryableSendFailure::PaymentExpired => PaymentFailureReason::PaymentExpired,


Is it worth implementing From<RetryableSendFailure> for PaymentFailureReason? Or wouldn't it be reusable outside of this context?

Hmmm... I considered it but it seemed a bit out of place importing RetryableSendFailure in the events module. Might be worth taking a look at how to we might better organize these errors given some of the duplication between RetryableSendFailure and Bolt12PaymentError along with the event reason.

tnull · 2024-07-12T10:02:19Z

lightning/src/ln/outbound_payment.rs

-							*payment.into_mut() = retryable_payment;
-							(total_amount, recipient_onion, None, onion_session_privs)
+						PendingOutboundPayment::InvoiceReceived { .. } => {
+							log_error!(logger, "Payment already initiating");


Should this log include the payment hash?

We could though, I'm wondering also if we should make this a log_trace. Given we'll send multiple invoice requests, we'll likely see multiple invoices even though we'll only handle the first one. The others may hit this case since the lock is released after transition to this state while path finding but before the payment is made.

Actually, I was mistaken. We should never hit this case just like PendingOutboundPayment::AwaitingInvoice. When we release the lock in send_payment_for_bolt12_invoice, the payment will have transitioned to PendingOutboundPayment::AwaitingInvoice. So attempting to handle another invoice with the same id will cause us to return Bolt12PaymentError::DuplicateInvoice.

Mhh, should this be a debug_assert then?

Done and did the same for PendingOutboundPayment::AwaitingInvoice.

jkczyz · 2024-07-12T14:59:33Z

Mostly looks good to me, just a few nits/questions.

It also seems this only addresses the first point of #3159, i.e., we currently don't early-abort before sending the invoice request if we're positive that we'll be unable to send the payment. Should we address this in another PR? In this case it might be worth opening a separate issue for it?

Yeah, let's do another PR for the second part. @slanesuke Do you think that's something you can look at? I'm juggling a few other tasks at the moment.

tnull · 2024-07-12T15:07:20Z

Yeah, let's do another PR for the second part. @slanesuke Do you think that's something you can look at? I'm juggling a few other tasks at the moment.

Cool, now moved the second part to this issue: #3174

slanesuke · 2024-07-12T15:25:02Z

Mostly looks good to me, just a few nits/questions.
It also seems this only addresses the first point of #3159, i.e., we currently don't early-abort before sending the invoice request if we're positive that we'll be unable to send the payment. Should we address this in another PR? In this case it might be worth opening a separate issue for it?

Yeah, let's do another PR for the second part. @slanesuke Do you think that's something you can look at? I'm juggling a few other tasks at the moment.

Yeah, I'll take care of it.

tnull

Feel free to squash the fixups

The BOLT11 and BOLT12 outbound payment initiation code differ in that the latter re-uses the retry path (i.e., find_route_and_send_payment). The drawback of this is that Ok is returned even if there is an error finding a route. Refactor send_payment_for_bolt12_invoice such that it re-uses find_initial_route instead so that errors can be returned.

tnull

LGTM

Landing this without second reviewer as the changeset is manageable.

TheBlueMatt

Post-merge LGTM

jkczyz added 2 commits July 11, 2024 18:01

Add Bolt12PaymentError::SendingFailed variant

aa55366

Instead of returning Ok when path finding fails, allow returning a RetryableSendFailure from send_payment_for_bolt12_invoice. Follow up commits will return such failures.

Refactor initial route finding to separate method

c8fd681

This will be used later in send_payment_for_bolt12_invoice instead of find_route_and_send_payment as it will allow for returning RetryableSendFailure when path finding fails.

jkczyz force-pushed the 2024-07-propagate-error branch from 900c10f to 2f087ab Compare July 11, 2024 23:18

tnull self-requested a review July 12, 2024 08:42

tnull reviewed Jul 12, 2024

View reviewed changes

tnull reviewed Jul 19, 2024

View reviewed changes

jkczyz added 2 commits July 19, 2024 08:47

Add debug_assert!(false) for unreachable code

b697fbe

jkczyz force-pushed the 2024-07-propagate-error branch from 579ebd1 to b697fbe Compare July 19, 2024 13:48

tnull approved these changes Jul 30, 2024

View reviewed changes

tnull merged commit 4a12b5f into lightningdevkit:main Jul 30, 2024
17 of 19 checks passed

TheBlueMatt reviewed Aug 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include routing failures in `Bolt12PaymentError` #3171

Include routing failures in `Bolt12PaymentError` #3171

jkczyz commented Jul 11, 2024

jkczyz commented Jul 11, 2024

codecov-commenter commented Jul 11, 2024 •

edited by codecov bot

Loading

tnull left a comment

tnull Jul 12, 2024

jkczyz Jul 18, 2024

tnull Jul 12, 2024

jkczyz Jul 18, 2024

jkczyz Jul 18, 2024

tnull Jul 18, 2024

jkczyz Jul 18, 2024

jkczyz commented Jul 12, 2024

tnull commented Jul 12, 2024

slanesuke commented Jul 12, 2024

tnull left a comment

tnull left a comment

TheBlueMatt left a comment

Include routing failures in Bolt12PaymentError #3171

Include routing failures in Bolt12PaymentError #3171

Conversation

jkczyz commented Jul 11, 2024

jkczyz commented Jul 11, 2024

codecov-commenter commented Jul 11, 2024 • edited by codecov bot Loading

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

tnull Jul 12, 2024

Choose a reason for hiding this comment

jkczyz Jul 18, 2024

Choose a reason for hiding this comment

tnull Jul 12, 2024

Choose a reason for hiding this comment

jkczyz Jul 18, 2024

Choose a reason for hiding this comment

jkczyz Jul 18, 2024

Choose a reason for hiding this comment

tnull Jul 18, 2024

Choose a reason for hiding this comment

jkczyz Jul 18, 2024

Choose a reason for hiding this comment

jkczyz commented Jul 12, 2024

tnull commented Jul 12, 2024

slanesuke commented Jul 12, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Include routing failures in `Bolt12PaymentError` #3171

Include routing failures in `Bolt12PaymentError` #3171

codecov-commenter commented Jul 11, 2024 •

edited by codecov bot

Loading