Expose a failure reason in `PaymentFailed` #2142

alecchendev · 2023-04-01T00:59:23Z

Closes #2095. Adds a new public enum PaymentFailureReason and a new field on Event::PaymentFailed to communicate why the payment failed completely.

alecchendev

Some things I’m considering changing right now if anyone wants to give some feedback:

lightning/src/events/mod.rs

lightning/src/ln/outbound_payment.rs

codecov-commenter · 2023-04-01T01:59:27Z

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.24 🎉

Comparison is base (f30dc85) 91.38% compared to head (23c7064) 91.62%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2142      +/-   ##
==========================================
+ Coverage   91.38%   91.62%   +0.24%     
==========================================
  Files         102      102              
  Lines       50288    52337    +2049     
  Branches    50288    52337    +2049     
==========================================
+ Hits        45954    47954    +2000     
- Misses       4334     4383      +49

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`92.38% <ø> (+3.02%)`	⬆️
lightning/src/events/mod.rs	`33.76% <37.50%> (+0.07%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.01% <77.77%> (-0.68%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`92.47% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.25% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`98.48% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.45% <100.00%> (+<0.01%)`	⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

This looks great, thanks!

lightning/src/events/mod.rs

lightning/src/ln/outbound_payment.rs

alecchendev · 2023-04-04T03:46:09Z

Squashed to pass CI and because it was getting messy to read all the fixups since I was changing/merging enum variants and had to undo work from previous commits.

lightning/src/events/mod.rs

lightning/src/ln/outbound_payment.rs

lightning/src/events/mod.rs

alecchendev · 2023-04-04T19:53:10Z

Separating my fixup commits so they're easier to squash later even though it'll fail CI how they currently are.

lightning/src/events/mod.rs

lightning/src/ln/outbound_payment.rs

valentinewallace

Looking good!

lightning/src/events/mod.rs

lightning/src/ln/outbound_payment.rs

valentinewallace · 2023-04-06T19:11:40Z

Feel free to squash!

valentinewallace

LGTM after squash

alecchendev · 2023-04-07T18:31:10Z

Rebased and fixed 2 lines for CI - payment_tests.rs line 2924 (the change here doesn't really relate to this PR, don't really know what's normal to do here) and 2959

TheBlueMatt · 2023-04-07T18:33:27Z

Oops that was just fixed in #2170.

alecchendev · 2023-04-07T18:48:47Z

Rebased again so it's not showing the overlapping change :)

lightning/src/ln/outbound_payment.rs

valentinewallace · 2023-04-10T16:27:39Z

lightning/src/ln/outbound_payment.rs

+				log_error!(logger, "Failed to send along path due to error: {:?}", e);
 				if let APIError::MonitorUpdateInProgress = e { continue }


Not a big deal, but ideally we shouldn't log_error if we get APIError::MonitorUpdateInProgress, because it's not an error (🙄). Could conditionally log or move the current log to after the continue

valentinewallace · 2023-04-10T16:31:33Z

lightning/src/ln/outbound_payment.rs

@@ -1232,7 +1226,7 @@ impl OutboundPayments {
 				} else {
 					PaymentFailureReason::RetriesExhausted
 				};
-				let _ = payment.get_mut().mark_abandoned(reason); // we'll only Err if it's a legacy payment
+				payment.get_mut().mark_abandoned(reason); // we'll only Err if it's a legacy payment


Can remove the comment

valentinewallace

LGTM after squash

This includes adding a reason to `PendingOutboundPayment::Abandoned` and using that reason when pushing an `Event::PaymentFailed`.

alecchendev commented Apr 1, 2023

View reviewed changes

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

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

TheBlueMatt reviewed Apr 3, 2023

View reviewed changes

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

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

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

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

TheBlueMatt added this to the 0.0.115 milestone Apr 3, 2023

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch 3 times, most recently from 387d455 to 0383f8b Compare April 4, 2023 03:41

alecchendev marked this pull request as ready for review April 4, 2023 03:48

valentinewallace reviewed Apr 4, 2023

View reviewed changes

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

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

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

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

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from fa682d4 to a58bfa6 Compare April 4, 2023 20:27

TheBlueMatt mentioned this pull request Apr 4, 2023

OutboundPayments::abandon_payment doesn't wait for the payment to fail to generate a PaymentFailed Event #2152

Closed

TheBlueMatt reviewed Apr 5, 2023

View reviewed changes

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from 4e7731d to 3b11ebb Compare April 5, 2023 19:58

TheBlueMatt assigned TheBlueMatt and valentinewallace Apr 5, 2023

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from 3b11ebb to caf991b Compare April 5, 2023 21:21

valentinewallace reviewed Apr 6, 2023

View reviewed changes

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

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

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

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from caf991b to 7cf9094 Compare April 7, 2023 01:09

valentinewallace reviewed Apr 7, 2023

View reviewed changes

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch 3 times, most recently from 207a3d4 to 17e8d4b Compare April 7, 2023 18:26

Create PaymentFailureReason enum

432f0e6

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from 17e8d4b to 4ba183d Compare April 7, 2023 18:46

TheBlueMatt reviewed Apr 7, 2023

View reviewed changes

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

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

valentinewallace reviewed Apr 10, 2023

View reviewed changes

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from 508d860 to 5251b39 Compare April 10, 2023 20:15

valentinewallace reviewed Apr 10, 2023

View reviewed changes

Add reason to Event::PaymentFailed

23c7064

This includes adding a reason to `PendingOutboundPayment::Abandoned` and using that reason when pushing an `Event::PaymentFailed`.

alecchendev force-pushed the 2023-03-expose-fail-reason-in-payment-failure branch from 5251b39 to 23c7064 Compare April 10, 2023 22:14

TheBlueMatt approved these changes Apr 11, 2023

View reviewed changes

valentinewallace approved these changes Apr 11, 2023

View reviewed changes

valentinewallace merged commit 157af6e into lightningdevkit:main Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose a failure reason in `PaymentFailed` #2142

Expose a failure reason in `PaymentFailed` #2142

alecchendev commented Apr 1, 2023

alecchendev left a comment

codecov-commenter commented Apr 1, 2023 •

edited

Loading

TheBlueMatt left a comment

alecchendev commented Apr 4, 2023

alecchendev commented Apr 4, 2023 •

edited

Loading

valentinewallace left a comment

valentinewallace commented Apr 6, 2023

valentinewallace left a comment

alecchendev commented Apr 7, 2023

TheBlueMatt commented Apr 7, 2023

alecchendev commented Apr 7, 2023

valentinewallace Apr 10, 2023

alecchendev Apr 10, 2023

valentinewallace Apr 10, 2023

valentinewallace left a comment

		log_error!(logger, "Failed to send along path due to error: {:?}", e);
		if let APIError::MonitorUpdateInProgress = e { continue }

Expose a failure reason in PaymentFailed #2142

Expose a failure reason in PaymentFailed #2142

Conversation

alecchendev commented Apr 1, 2023

alecchendev left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 1, 2023 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

alecchendev commented Apr 4, 2023

alecchendev commented Apr 4, 2023 • edited Loading

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace commented Apr 6, 2023

valentinewallace left a comment

Choose a reason for hiding this comment

alecchendev commented Apr 7, 2023

TheBlueMatt commented Apr 7, 2023

alecchendev commented Apr 7, 2023

valentinewallace Apr 10, 2023

Choose a reason for hiding this comment

alecchendev Apr 10, 2023

Choose a reason for hiding this comment

valentinewallace Apr 10, 2023

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Expose a failure reason in `PaymentFailed` #2142

Expose a failure reason in `PaymentFailed` #2142

codecov-commenter commented Apr 1, 2023 •

edited

Loading

alecchendev commented Apr 4, 2023 •

edited

Loading