Rework auto-retry send errors #2014

valentinewallace · 2023-02-06T21:18:59Z

Partially addresses #1932. Fixes #1969.

Previously we would return a PaymentSendFailure from auto-retry send_payment methods. This doesn't make sense because they will never return the PartialFailure variant, so we add a new error enum for them here.

We now also generate PaymentPathFailed events for paths that fail on the first hop before they can commit to the htlc (eg if we tried to send more than we have in the channel).

~~Based on #2008~~

valentinewallace · 2023-02-06T21:20:29Z

@TheBlueMatt I'm wondering if generating PaymentPathFailed events on partial/all-paths failure should be release blocking, since we already don't support this in InvoicePayer. I guess I might not be clear on what those events will be used for -- ISTM they're basically just telling the user "your router is buggy"?

codecov-commenter · 2023-02-14T00:15:56Z

Codecov Report

Base: 87.20% // Head: 87.89% // Increases project coverage by +0.68% 🎉

Coverage data is based on head (a6e9123) compared to base (558b2f2).
Patch coverage: 78.23% of modified lines in pull request are covered.

📣 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    #2014      +/-   ##
==========================================
+ Coverage   87.20%   87.89%   +0.68%     
==========================================
  Files         101      101              
  Lines       44175    49323    +5148     
  Branches    44175    49323    +5148     
==========================================
+ Hits        38525    43351    +4826     
- Misses       5650     5972     +322

Impacted Files	Coverage Δ
lightning-invoice/src/payment.rs	`75.75% <0.00%> (ø)`
lightning/src/ln/outbound_payment.rs	`79.13% <73.39%> (-0.83%)`	⬇️
lightning/src/ln/payment_tests.rs	`95.65% <93.75%> (-0.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`89.56% <100.00%> (+3.75%)`	⬆️
lightning/src/util/time.rs	`38.29% <0.00%> (-4.56%)`	⬇️
lightning/src/ln/features.rs	`92.55% <0.00%> (-0.33%)`	⬇️
lightning/src/util/errors.rs	`32.14% <0.00%> (ø)`
lightning/src/routing/gossip.rs	`84.04% <0.00%> (+0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.33% <0.00%> (+0.04%)`	⬆️
lightning/src/routing/router.rs	`92.84% <0.00%> (+0.04%)`	⬆️
... and 13 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/util/events.rs

TheBlueMatt · 2023-02-16T23:57:01Z

lightning/src/util/events.rs

 						(7, short_channel_id, option),
 						(9, retry, option),
 						(11, payment_id, option),
+						(13, failure_opt, ignorable),


Doesn't the ignorable field imply that if its missing we'll return None for the full event if its missing? Also a bug for the network_update field, right?

The expanded macro code looks good to me. I think we'll only return None for the full event if the MaybeReadable::read returns Ok(None), which only happens if the TLV type for the field is present.

Ah, hmm, I was thinking of the ignorable case in _init_tlv_based_struct_field. This is confusing - we handle ignorable in the higher-level macros as "implement MaybeReadable and return Ok(None)" but in the lower-level macros as just a regular ol' option. Can we make it consistent - use the new ignorable_option as a regular ol' option but make the ignorable be a required field in the lower-level macros as well, returning Ok(None) in _check_missing_tlv (which implies we're in a MaybeReadable, and maybe renaming the ignorable to ignorable_no_read or whatever?

maybe renaming the ignorable to ignorable_no_read or whatever?

That all sounds good, though I'd also be in favor of renaming ignorable_* to upgradable_*. Because I'd like to name the above variant ignorable_required, but that sounds like an oxymoron.

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2023-02-17T00:19:52Z

lightning/src/util/events.rs

+pub enum Failure {
+	/// We failed to initially send the payment and no HTLC was committed to. Contains a relevant
+	/// error message.
+	InitialSend(String),


I think in ~every case we have exactly one APIError to put here, why not that instead of a String?

It doesn't implement writeable at the moment, though that wouldn't be too hard to add.

Yea, let's just do that.

FYI, I think we'll need to change the APIError::InvalidRoute to contain a String. I don't think we can implement Readable for a &'static str because it references data owned by the read function.

TheBlueMatt · 2023-02-17T00:21:51Z

@TheBlueMatt I'm wondering if generating PaymentPathFailed events on partial/all-paths failure should be release blocking

Given where other things are it seems we have time? Its not exactly critical, but if I'm tracking failures/successes I'd like to see a failure for every Route path my Router returns, I think.

valentinewallace · 2023-02-17T16:09:33Z

@TheBlueMatt I'm wondering if generating PaymentPathFailed events on partial/all-paths failure should be release blocking

Given where other things are it seems we have time? Its not exactly critical, but if I'm tracking failures/successes I'd like to see a failure for every Route path my Router returns, I think.

Yeah agreed, that comment was out of date sorry

Useful for generating Payment(Path)Failed events in this method

valentinewallace · 2023-02-20T18:56:43Z

Tacked on a small commit to fix #1969. This PR ended up taking on some unrelated changes, so I'd be happy to split it up if reviewers prefer.

TheBlueMatt

Yea, let's definitely try to split this up, maybe everything after Fix outdated PendingOutboundPayment::Abandoned docs can go into separate (probably two?) PRs?

TheBlueMatt · 2023-02-21T17:30:22Z

lightning/src/util/events.rs

 					let mut payment_hash = PaymentHash([0; 32]);
 					let mut payment_failed_permanently = false;
 					let mut network_update = None;
-					let mut all_paths_failed = Some(true);


It looks like the backwards compat approach when this was added was that we we didnt support MPP at all so we could assume this was always true. For forwards compat, though, this doesn't make sense - if someone downgrades to 113 from 114 and looks at this field it will always report that all HTLCs have failed, which is obviously nonsense. Its been long enough since we added the abandon flow that we can break forwards compat here, I think, but its a bit safer to write Some(false) rather than letting old clients assume true by writing None. We also need to add a draft release note here that says that the field has been removed and old readers will always see false, and users wishing to support downgrade who still rely on the field should instead first migrate to the abandon-based approach and look at PaymentFailed events and then migrate to 114.

Will address this in #2043

Edit: addressed

valentinewallace · 2023-02-21T18:17:33Z

Split up the PR, see #2043

lightning/src/ln/outbound_payment.rs

jkczyz · 2023-02-21T23:13:46Z

lightning/src/ln/outbound_payment.rs

 			},
 			PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. } => {
-				Self::push_payment_path_failed_evs(payment_id, payment_hash, route.paths, results.into_iter(), pending_events);
+				Self::push_payment_path_failed_evs(payment_id, payment_hash, None, route.paths, results.into_iter(), pending_events);


Why None here?

We could push the failed scids here, but they won't be used because we just use the PartialFailure::failed_paths_retry for resend, which already contains them.

But shouldn't the event that we surface also reflect this? Why would we exclude the scid here but include it for the AllFailedResendSafe case?

More generally, I guess it's hard to follow where previously_failed_channels is populated. In this function, it is a side effect of calling push_payment_path_failed_evs, but only for AllFailedResendSafe. Then when looking at fail_htlc for a non-immediate failure, it looks to be added twice. Once when calling insert_previously_failed_scid and another time when determining the failure was not a probe. Is that expected or am I misreading that? Maybe the second insert is necessary for the backwards compatibility case? Just thinking if there s there any way we could have it added in a single place.

when looking at fail_htlc for a non-immediate failure, it looks to be added twice. Once when calling insert_previously_failed_scid and another time when determining the failure was not a probe.

So the second add will go away when we get rid of PaymentPathFailed::retry, which as you pointed out should be removed.

But shouldn't the event that we surface also reflect this? Why would we exclude the scid here but include it for the AllFailedResendSafe case?

We never include failed scids in the event surfaced in handle_pay_route_err, PaymentPathFailed::retry is always None, if I understand you correctly. The scids are being pushed into the route params for immediate retry purposes only.

On a related note, on second glance my statement that we push failed scids into failed_paths_retry on PartialFailure was not correct. I think I thought that because the issue says it: #1969 but it isn't in the code. Fixing this.

Ah, there was also a rebase error, push_payment_path_failed_evs should be renamed to push_path_failed_evs_and_scids. Will also fix this

Edit: fixed this and above

We never include failed scids in the event surfaced in handle_pay_route_err, PaymentPathFailed::retry is always None, if I understand you correctly. The scids are being pushed into the route params for immediate retry purposes only.

Ah, right, I missed that. Was thinking it was added from the parameter.

jkczyz · 2023-02-21T23:25:11Z

lightning/src/ln/outbound_payment.rs

+/// If a payment fails to send with [`ChannelManager::send_payment`], it can be in one of several
+/// states. This enum is returned as the Err() type describing which state the payment is in, see
+/// the description of individual enum states for more.
+///
+/// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment
 #[derive(Clone, Debug)]
 pub enum PaymentSendFailure {


Do we plan to deprecate ChannelManager::send_payment?

I don't know if we've fully decided since it doesn't hurt, but it would definitely clean up this module a bit and the manual route case is already supported by the Router trait anyway.

lightning/src/ln/outbound_payment.rs

jkczyz · 2023-02-22T18:46:27Z

lightning/src/ln/outbound_payment.rs

 			},
 			PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. } => {
-				Self::push_payment_path_failed_evs(payment_id, payment_hash, route.paths, results.into_iter(), pending_events);
+				Self::push_payment_path_failed_evs(payment_id, payment_hash, None, route.paths, results.into_iter(), pending_events);


We never include failed scids in the event surfaced in handle_pay_route_err, PaymentPathFailed::retry is always None, if I understand you correctly. The scids are being pushed into the route params for immediate retry purposes only.

Ah, right, I missed that. Was thinking it was added from the parameter.

TheBlueMatt · 2023-02-23T00:51:31Z

Feel free to squash, IMO.

jkczyz · 2023-02-23T04:04:09Z

Feel free to squash, IMO.

Same! LGTM

InvalidRoute is reserved for malformed routes, not routes where a channel or its peer is unavailable

valentinewallace · 2023-02-23T04:08:51Z

Squashed!

jkczyz

Largely looks good minus a couple things that I noticed.

lightning/src/ln/outbound_payment.rs

jkczyz · 2023-02-23T15:43:26Z

lightning/src/ln/outbound_payment.rs

+		&self, payment_id: PaymentId, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+		keysend_preimage: Option<PaymentPreimage>, retry_strategy: Retry, route_params: RouteParameters,


Parameters are a little inconsistent here. Should we pass payment_secret by value?

It's preexisting and I think it would be better to fix it all in one go, since ChannelManager::send_payment and friends also need this fix

freddiekrugerrand

Reviewed to the best of my ability with limited understanding of ldk/rust.
Nice refinement of payment states for retries!

freddiekrugerrand · 2023-02-22T16:16:54Z

lightning/src/ln/outbound_payment.rs

-				return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
-					err: format!("Invoice expired for payment id {}", log_bytes!(payment_id.0)),
-				}))
+				return Err(RetryableSendFailure::PaymentExpired)


If this check means that the invoice is expired, does it make more sense to rename PaymentExpired -> InvoiceExpired? Assuming that there's also a sender-set time limit on how long we attempt a payment for, I'd think that time running out is a payment expiry.

PaymentExpired is definitely confusing since we also have Retry::Timeout which is a different thing. Since the PaymentParameters::expiry_time technically doesn't need to be from an invoice (it could even be set for spontaneous payments), I ended up clarifying the docs on this error variant and on Retry::Timeout

freddiekrugerrand · 2023-02-23T18:08:50Z

lightning/src/ln/outbound_payment.rs

+			Some(&first_hops.iter().collect::<Vec<_>>()), &inflight_htlcs()
+		).map_err(|_| RetryableSendFailure::RouteNotFound)?;
+
+		let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret,


If add_new_pending_payment errors are always mapped to DuplicatePayment, can it be updated to return a RetryableSendFailure instead?

Ah, it isn't always mapped to that, but yeah that would be nicer.

Prior to this, we returned PaymentSendFailure from auto retry send payment methods. This implied that we might return a PartialFailure from them, which has never been the case. So it makes sense to rework the errors to be a better fit for the methods. We're taking error handling in a totally different direction now to make it more asynchronous, see send_payment_internal for more information.

Since it's only used one place now

Previously, we could have tried the same failed channels over and over until retries are exhausted.

TheBlueMatt

Please fix in a followup.

TheBlueMatt · 2023-02-23T21:53:57Z

lightning/src/ln/outbound_payment.rs

+		debug_assert_eq!(paths.len(), path_results.len());
+		for (path, path_res) in paths.into_iter().zip(path_results) {
+			if let Err(e) = path_res {
+				let failed_scid = if let APIError::InvalidRoute { .. } = e {


I think we need to not push a failure event if the error is APIError::MonitorUpdateInProgress.

valentinewallace added the blocked on dependent pr label Feb 6, 2023

TheBlueMatt mentioned this pull request Feb 7, 2023

Abandon payments on behalf of the user and remove manual retries #2008

Merged

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from 22698fc to 8f1a5db Compare February 13, 2023 23:30

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from 8f1a5db to c331f58 Compare February 14, 2023 00:17

valentinewallace changed the title ~~WIP: Rework auto-retry send errors~~ Rework auto-retry send errors Feb 14, 2023

valentinewallace commented Feb 14, 2023

View reviewed changes

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

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch 3 times, most recently from 9946121 to 6513398 Compare February 16, 2023 20:53

valentinewallace marked this pull request as ready for review February 16, 2023 20:56

valentinewallace mentioned this pull request Feb 16, 2023

Move Payment Retrying into ChannelManager #1932

Closed

10 tasks

TheBlueMatt reviewed Feb 17, 2023

View reviewed changes

valentinewallace added 2 commits February 19, 2023 18:01

Pass payment hash into pay_internal

cb81d27

Useful for generating Payment(Path)Failed events in this method

Pass pending_events into pay_internal

9f41bd7

Useful for generating Payment(Path)Failed events in this method

valentinewallace removed the blocked on dependent pr label Feb 20, 2023

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch 2 times, most recently from 1f272c0 to 8492847 Compare February 20, 2023 18:35

valentinewallace added the Seeking Code Review label Feb 20, 2023

TheBlueMatt reviewed Feb 21, 2023

View reviewed changes

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from 107e6e1 to 2b14430 Compare February 21, 2023 18:07

valentinewallace mentioned this pull request Feb 21, 2023

PaymentPathFailed: add initial send error details #2043

Merged

jkczyz self-requested a review February 21, 2023 20:30

jkczyz removed the Seeking Code Review label Feb 21, 2023

TheBlueMatt reviewed Feb 21, 2023

View reviewed changes

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

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

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

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

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from 2b14430 to ed1ef50 Compare February 21, 2023 23:00

jkczyz reviewed Feb 21, 2023

View reviewed changes

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from ed1ef50 to 7698078 Compare February 22, 2023 17:37

jkczyz reviewed Feb 22, 2023

View reviewed changes

TheBlueMatt added this to the 0.0.114 milestone Feb 22, 2023

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch 2 times, most recently from fea92d5 to d086918 Compare February 22, 2023 20:50

Fix InvalidRoute error to be ChannelUnavailable

5e4f0bc

InvalidRoute is reserved for malformed routes, not routes where a channel or its peer is unavailable

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from d086918 to dc05c6d Compare February 23, 2023 04:07

jkczyz reviewed Feb 23, 2023

View reviewed changes

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from dc05c6d to 6b4ce57 Compare February 23, 2023 16:36

jkczyz previously approved these changes Feb 23, 2023

View reviewed changes

freddiekrugerrand previously approved these changes Feb 23, 2023

View reviewed changes

valentinewallace added 5 commits February 23, 2023 15:50

In-line retry_with_route method

b826d17

Since it's only used one place now

On initial send retries, avoid previously failed scids

1224dac

Previously, we could have tried the same failed channels over and over until retries are exhausted.

Fix outdated PendingOutboundPayment::Abandoned docs

12bcc9a

Clarify Retry::Timeout vs PaymentParams::expiry_time in docs

a6e9123

valentinewallace dismissed stale reviews from freddiekrugerrand and jkczyz via a6e9123 February 23, 2023 20:51

valentinewallace force-pushed the 2023-02-rework-partial-pmt-fail branch from 6b4ce57 to a6e9123 Compare February 23, 2023 20:51

jkczyz approved these changes Feb 23, 2023

View reviewed changes

TheBlueMatt approved these changes Feb 23, 2023

View reviewed changes

TheBlueMatt merged commit 46fd703 into lightningdevkit:main Feb 23, 2023

		&self, payment_id: PaymentId, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
		keysend_preimage: Option<PaymentPreimage>, retry_strategy: Retry, route_params: RouteParameters,

Rework auto-retry send errors #2014

Rework auto-retry send errors #2014

Uh oh!

Conversation

valentinewallace commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace commented Feb 6, 2023

Uh oh!

codecov-commenter commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 17, 2023

Uh oh!

valentinewallace commented Feb 17, 2023

Uh oh!

valentinewallace commented Feb 20, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Feb 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 23, 2023

Uh oh!

jkczyz commented Feb 23, 2023

Uh oh!

valentinewallace commented Feb 23, 2023

Uh oh!

jkczyz left a comment

valentinewallace commented Feb 6, 2023 •

edited

Loading

codecov-commenter commented Feb 14, 2023 •

edited

Loading

valentinewallace Feb 21, 2023 •

edited

Loading

valentinewallace Feb 22, 2023 •

edited

Loading

freddiekrugerrand left a comment •

edited

Loading