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

valentinewallace · 2023-02-03T18:01:17Z

Since ChannelManager now parameterizes payment sends with a retry strategy, we're removing support for manual retries.

As a side effect of this, it makes sense to always call abandon_payment on behalf of the user.

Partially addresses #1932

Based on #2007

jkczyz · 2023-02-03T19:03:52Z

Concept ACK

TheBlueMatt · 2023-02-03T19:15:52Z

Can be rebased now. Some further doc updates will need to happen based on the changes from #1873.

codecov-commenter · 2023-02-06T02:27:30Z

Codecov Report

Base: 91.07% // Head: 91.07% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (485809d) compared to base (ce2ed44).
Patch coverage: 91.08% of modified lines in pull request are covered.

❗ Current head 485809d differs from pull request most recent head d6f805c. Consider uploading reports for the commit d6f805c to get more accurate results

📣 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    #2008   +/-   ##
=======================================
  Coverage   91.07%   91.07%           
=======================================
  Files          99       99           
  Lines       51711    51679   -32     
  Branches    51711    51679   -32     
=======================================
- Hits        47097    47068   -29     
+ Misses       4614     4611    -3

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`94.91% <ø> (ø)`
lightning/src/util/events.rs	`31.63% <ø> (+2.07%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.54% <80.00%> (-0.05%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.93% <82.60%> (-0.06%)`	⬇️
lightning/src/ln/payment_tests.rs	`97.00% <92.50%> (-0.25%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`91.38% <93.75%> (-0.09%)`	⬇️
lightning/src/ln/outbound_payment.rs	`90.58% <98.46%> (+1.20%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.40% <100.00%> (-0.04%)`	⬇️
lightning/src/ln/monitor_tests.rs	`99.56% <100.00%> (-0.01%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.40% <100.00%> (-0.01%)`	⬇️
... and 6 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.

valentinewallace · 2023-02-06T18:20:38Z

~~I can't repro the CI fuzz failure locally. I'm running:~~

export TARGET="chanmon_consistency" # adjust for your output
export HEX="32fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0\
3641404140757453743563747970654963456253743563747970"

mkdir -p ./test_cases/$TARGET
echo $HEX | xxd -r -p > ./test_cases/$TARGET/any_filename_works

export RUST_BACKTRACE=1
export RUSTFLAGS="--cfg=fuzzing"
cargo test

~~from the fuzz directory. Lmk if someone sees something wrong with this script 👀~~

Edit: I added +1.58 to the cargo test line and was able to repro

TheBlueMatt

Didnd't review the test changes yet.

TheBlueMatt · 2023-02-07T21:17:58Z

lightning/src/ln/outbound_payment.rs

 				if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), inflight_htlcs(), entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) {
 					log_info!(logger, "Errored retrying payment: {:?}", e);
+					self.abandon_payment(payment_id, pending_events);


I'd appreciate a nice, long comment here and doc comment on pay_internal talking about what cases pay_internal is guaranteed to return an Err on and why we okay using that as an indicator here - the current Err-or-not logic is a bit haphazard so would like it in writing.

Take a look at the new docs pls

This is going to change a lot in #2014 actually, so I'm going to save this feedback item for that PR

lightning/src/ln/outbound_payment.rs

alecchendev

Mainly took a look at channelmanager.rs and outbound_payment.rs to learn and ask questions. Looks good as far as I can tell!

I saw in related PRs that moving payment retrying into ChannelManager is to enable support for trampoline payments. I was wondering how it does that. Are all these PRs to support automated retrying of payments for when a node built with LDK is acting as a trampoline node?

alecchendev · 2023-02-09T05:15:12Z

lightning/src/ln/outbound_payment.rs

+			outbounds.retain(|pmt_id, pmt| {
+				let mut retain = true;
+				let auto_retryable = pmt.is_auto_retryable_now();
+				if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt {


I'm confused what's going on in this loop (outbounds.retain(...)). For each outbound payment, it looks to be checking if it's valid to retry, and either marking it abandoned and pushing a PaymentFailed event or setting retry_id_route_params to retry the payment. I'm mainly confused because it looks like if there's multiple retryable payments, this will just set retry_id_route_params multiple times and afterwards only retry whatever single payment it was last set to. I'd imagine it'd make more sense want to retry each retryable payment that it finds from this iteration (instead of looping through all payments to just retry a single one) right? Not sure if I'm understanding this correctly

This was something I meant to revisit but forgot, thanks. I'm still not 100% sure of the new solution since it adds an extra lock of pending_outbound_payments, but it seems overall better to me for the reasons you specified.

We can't retry each payment as we go because we need to drop the pending_outbound_payments lock each time we call pay_internal. It is a bit weird though. See this discussion for more info: #1916 (comment)

We can't retry each payment as we go because we need to drop the pending_outbound_payments lock each time we call pay_internal. It is a bit weird though. See this discussion for more info: #1916 (comment)

Ah gotcha.

I think the new solution is an improvement at least in readability with the iteration for retrying/lock dropping and abandoning of payments being separate, but yea, this seems tough to do in a clean way :)

alecchendev · 2023-02-09T05:24:25Z

lightning/src/ln/outbound_payment.rs

+		{
+			let mut pending_outbounds = outbound_payments.pending_outbound_payments.lock().unwrap();
+			if let Some(PendingOutboundPayment::Retryable { ref mut total_msat, .. }) = pending_outbounds.get_mut(&our_payment_id) {
+				*total_msat += 1000;
+			}
+		}


Confused what this block is for. I'm thinking this whole test to make sure that check_retry_payments abandons + fails the payment after exhausting retry attempts...is this part to simulate that there are more sats that still need to be sent than are pending..?

is this part to simulate that there are more sats that still need to be sent than are pending..?

Yup, exactly. Added a comment to clarify

alecchendev · 2023-02-09T05:28:52Z

lightning/src/ln/outbound_payment.rs

+	pub(super) fn abandon_payment(&self, payment_id: PaymentId,
+		pending_events: &Mutex<Vec<events::Event>>
+	) {


Just curious what the reason is for changing abandon_payment, check_retry_payments, etc. to take in and modify pending_events as opposed to how it was before. Is it mainly just because it’s a bit cleaner or something else?

It's a bit cleaner because we want to keep as much code out of channelmanager.rs as possible, since it's already huge :)

valentinewallace · 2023-02-09T18:49:15Z

Are all these PRs to support automated retrying of payments for when a node built with LDK is acting as a trampoline node?

Yes, that's exactly right!

jkczyz

Sorry for the delay. Github wan't very functional on airline wifi. 😞

lightning/src/ln/outbound_payment.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/outbound_payment.rs

jkczyz · 2023-02-11T20:21:40Z

lightning/src/util/events.rs

+		/// Parameters used by LDK to compute a new [`Route`] when retrying the failed payment path.
 		///
 		/// [`Route`]: crate::routing::router::Route
-		/// [`find_route`]: crate::routing::router::find_route
 		retry: Option<RouteParameters>,


Do we need these anymore?

No, though from offline discussions this doesn't need to block the release. Documented in follow-up issue

lightning/src/util/events.rs

jkczyz

Largely LGTM modulo a question about the test changes.

lightning/src/ln/outbound_payment.rs

jkczyz · 2023-02-14T22:52:26Z

lightning/src/ln/functional_test_utils.rs

+			if i == expected_paths.len() - 1 {
+				match events[1] {
+					Event::PaymentFailed { ref payment_hash, ref payment_id } => {
+						assert_eq!(*payment_hash, our_payment_hash, "unexpected second payment_hash");
+						assert_eq!(*payment_id, expected_payment_id);
+					}
+					_ => panic!("Unexpected second event"),
+				}
+			}


Not sure I completely understand the changes to the testing behavior. Not necessarily here -- which may be justified -- but elsewhere where the number of events has increased because of the additional Event::PaymentFailed generated. Wouldn't it be less invasive if the Event::PaymentFailed case was only checked in a dedicated test? Then when testing other behavior, we could have retries remaining such that no new event are generated so those tests are more narrowly focused.

A portion of the changed tests won't have retryable HTLCs, so the PaymentFailed will be generated no matter what. Most of the others that I'm seeing tend to use the test utils such as route_payment, but when I update that util to use _with_retry I get 55 test failures.. I think it might be tricky since having retryable failed HTLCs will often lead to the generation of retry msg events to be handled. That said, it would probably be good in the long term to move the test utils to use _with_retry

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2023-02-14T22:47:59Z

lightning/src/ln/outbound_payment.rs

@@ -1048,6 +1061,10 @@ impl OutboundPayments {
 				});
 			}

+			if !payment_is_probe && (!is_retryable_now || !payment_retryable || retry.is_none()) {


Wait, its not a new issue, but where do we remove pending payments for probes? AFAICT we add one normally in send_probe but then don't remove it anywhere here.

Good point, filed #2036

lightning/src/ln/payment_tests.rs

lightning/src/ln/outbound_payment.rs

lightning/src/ln/functional_test_utils.rs

This makes it uniform with the outbound payment methods that generate events and set us up for abandoning payments on behalf of the user.

TheBlueMatt · 2023-02-15T20:27:22Z

Can you add this to Abandon payment if retry fails in process_pending_htlcs? I think it describes correctly the return conditions which the new code is ultimately relying on.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 52208bcc2..d1749bf5f 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -546,6 +546,7 @@ impl OutboundPayments {
                });
        }
 
+       /// Will return `Ok(())` iff at least one HTLC is sent for the payment.
        fn pay_internal<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>(
                &self, payment_id: PaymentId,
                initial_send_info: Option<(PaymentHash, &Option<PaymentSecret>, Option<PaymentPreimage>, Retry)>,

lightning/src/ln/payment_tests.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-02-15T22:33:04Z

lightning/src/util/events.rs

@@ -670,20 +664,6 @@ pub enum Event {
 		/// For both single-path and multi-path payments, this is set if all paths of the payment have
 		/// failed. This will be set to false if (1) this is an MPP payment and (2) other parts of the
 		/// larger MPP payment were still in flight when this event was generated.
-		///
-		/// Note that if you are retrying individual MPP parts, using this value to determine if a


Let's just remove the field entirely rather than just this hunk of text?

It's removed in #2014, let me know if you'd prefer we move that commit over here

lightning/src/ln/outbound_payment.rs

Removed retry_single_path_payment, it's replaced by automatic_retries with AutoRetry::Success

We're no longer supporting manual retries since ChannelManager::send_payment_with_retry can be parameterized by a retry strategy This commit also updates all docs related to retry_payment and abandon_payment. Since these docs frequently overlap with changes in preceding commits where we start abandoning payments on behalf of the user, all the docs are updated in one go.

TheBlueMatt added this to the 0.0.114 milestone Feb 3, 2023

valentinewallace force-pushed the 2023-02-remove-manual-retries branch 2 times, most recently from aa81d37 to 07c3ed9 Compare February 3, 2023 19:57

valentinewallace mentioned this pull request Feb 3, 2023

Replace InvoicePayer with utility methods #2007

Merged

valentinewallace force-pushed the 2023-02-remove-manual-retries branch from 6a1f42f to 2d4c713 Compare February 6, 2023 02:07

valentinewallace marked this pull request as ready for review February 6, 2023 02:12

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

valentinewallace mentioned this pull request Feb 6, 2023

Rework auto-retry send errors #2014

Merged

valentinewallace force-pushed the 2023-02-remove-manual-retries branch 2 times, most recently from 7514dfc to 485809d Compare February 7, 2023 19:22

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

TheBlueMatt added the Seeking Code Review label Feb 7, 2023

TheBlueMatt reviewed Feb 7, 2023

View reviewed changes

valentinewallace force-pushed the 2023-02-remove-manual-retries branch from 485809d to d6f805c Compare February 9, 2023 01:23

alecchendev reviewed Feb 9, 2023

View reviewed changes

valentinewallace force-pushed the 2023-02-remove-manual-retries branch 2 times, most recently from ae2f238 to 2fab2b8 Compare February 9, 2023 18:48

jkczyz reviewed Feb 11, 2023

View reviewed changes

valentinewallace mentioned this pull request Feb 11, 2023

Deduplicate PendingHTLCsForwardable events on generation #2026

Merged

valentinewallace force-pushed the 2023-02-remove-manual-retries branch from 2fab2b8 to a97e1d2 Compare February 13, 2023 16:46

wpaulino removed the Seeking Code Review label Feb 13, 2023

jkczyz reviewed Feb 14, 2023

View reviewed changes

TheBlueMatt reviewed Feb 14, 2023

View reviewed changes

valentinewallace added 2 commits February 14, 2023 21:29

Fix indentation in outbound payment mark_abandoned

8d0c08c

Pass pending events to outbound_payments::abandon_payment

f871210

This makes it uniform with the outbound payment methods that generate events and set us up for abandoning payments on behalf of the user.

valentinewallace force-pushed the 2023-02-remove-manual-retries branch from a97e1d2 to 03b5e8a Compare February 15, 2023 17:42

Abandon payment if retry fails in process_pending_htlcs

07dd8b2

valentinewallace force-pushed the 2023-02-remove-manual-retries branch from 03b5e8a to 42c2350 Compare February 15, 2023 20:55

valentinewallace mentioned this pull request Feb 15, 2023

Fix outbound payments probe leak #2037

Merged

TheBlueMatt reviewed Feb 15, 2023

View reviewed changes

valentinewallace added 4 commits February 15, 2023 17:46

Abandon payment on behalf of the user on payment path failure

82e0880

Removed retry_single_path_payment, it's replaced by automatic_retries with AutoRetry::Success

When processing pending htlcs, abandon outbounds that are not retryable

13e60da

Reword and fix grammar in PartialFailure docs

da34ada

valentinewallace force-pushed the 2023-02-remove-manual-retries branch from 42c2350 to da34ada Compare February 15, 2023 22:59

TheBlueMatt approved these changes Feb 15, 2023

View reviewed changes

jkczyz approved these changes Feb 16, 2023

View reviewed changes

TheBlueMatt merged commit 30b9d9f into lightningdevkit:main Feb 16, 2023

wpaulino mentioned this pull request Mar 3, 2023

Cut 0.0.114 #2065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valentinewallace commented Feb 3, 2023 •

edited

Loading

jkczyz commented Feb 3, 2023

TheBlueMatt commented Feb 3, 2023

codecov-commenter commented Feb 6, 2023 •

edited

Loading

valentinewallace commented Feb 6, 2023 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Feb 7, 2023

valentinewallace Feb 9, 2023

valentinewallace Feb 13, 2023

alecchendev left a comment

alecchendev Feb 9, 2023

valentinewallace Feb 9, 2023

alecchendev Feb 9, 2023 •

edited

Loading

alecchendev Feb 9, 2023

valentinewallace Feb 9, 2023

alecchendev Feb 9, 2023

valentinewallace Feb 9, 2023

valentinewallace commented Feb 9, 2023

jkczyz left a comment

jkczyz Feb 11, 2023

valentinewallace Feb 13, 2023

jkczyz left a comment

jkczyz Feb 14, 2023

valentinewallace Feb 15, 2023

TheBlueMatt Feb 14, 2023

valentinewallace Feb 15, 2023

TheBlueMatt commented Feb 15, 2023

TheBlueMatt Feb 15, 2023

valentinewallace Feb 15, 2023

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

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

Conversation

valentinewallace commented Feb 3, 2023 • edited Loading

jkczyz commented Feb 3, 2023

TheBlueMatt commented Feb 3, 2023

codecov-commenter commented Feb 6, 2023 • edited Loading

Codecov Report

valentinewallace commented Feb 6, 2023 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecchendev Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Feb 9, 2023

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Feb 3, 2023 •

edited

Loading

codecov-commenter commented Feb 6, 2023 •

edited

Loading

valentinewallace commented Feb 6, 2023 •

edited

Loading

alecchendev Feb 9, 2023 •

edited

Loading