1916 Followups Part 1 #1994

TheBlueMatt · 2023-01-28T05:58:36Z

While digging into followups for #1916 I noticed the amount issue, and then also noticed the CLTV issue which is existing in our retry pipeline.

codecov-commenter · 2023-01-28T06:23:13Z

Codecov Report

Base: 90.91% // Head: 91.32% // Increases project coverage by +0.40% 🎉

Coverage data is based on head (172241a) compared to base (d4de913).
Patch coverage: 95.66% of modified lines in pull request are covered.

❗ Current head 172241a differs from pull request most recent head face191. Consider uploading reports for the commit face191 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    #1994      +/-   ##
==========================================
+ Coverage   90.91%   91.32%   +0.40%     
==========================================
  Files          99       99              
  Lines       52505    55081    +2576     
  Branches    52505    55081    +2576     
==========================================
+ Hits        47735    50302    +2567     
- Misses       4770     4779       +9

Impacted Files	Coverage Δ
lightning/src/ln/outbound_payment.rs	`90.03% <91.25%> (+0.21%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`91.47% <94.33%> (+0.26%)`	⬆️
lightning/src/routing/router.rs	`91.13% <94.44%> (-0.02%)`	⬇️
lightning/src/ln/payment_tests.rs	`97.20% <97.23%> (-0.05%)`	⬇️
lightning-invoice/src/payment.rs	`89.65% <100.00%> (+0.01%)`	⬆️
lightning-invoice/src/utils.rs	`98.40% <100.00%> (+0.78%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.25% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.89% <100.00%> (-0.18%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.41% <100.00%> (ø)`
lightning/src/ln/priv_short_conf_tests.rs	`96.53% <100.00%> (ø)`
... and 19 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/ln/outbound_payment.rs

lightning/src/ln/payment_tests.rs

valentinewallace · 2023-01-30T20:37:46Z

lightning/src/ln/outbound_payment.rs

-							pmt.increment_attempts();
+							let mut params = params.clone();
+							if params.max_path_count as usize > retries_remaining {
+								params.max_path_count = retries_remaining as u8;


Why do we want to do this? I read the max_path_count docs as the max paths per payment attempt, not total including retries

Right, in this case there is one path remaining that we're willing to take for this payment (attempt), so we should definitely limit the path finder to ensure we only get that many paths.

This ultimately stems from the (admittedly somewhat strange) decision to tie retries to the number of payment paths in retry attempts, rather than the number of failed paths in the being-retried HTLCs. It left the code a good chunk cleaner (as it lets us increment the attempts in one place, in retry_payment_with_route, rather than keeping track of whether an attempt is for a retry or original path, and then incrementing in PendingOutboundPayment::remove)

valentinewallace · 2023-01-30T20:39:39Z

lightning/src/ln/outbound_payment.rs

 		&self, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId,
 		retry_strategy: Retry, route_params: RouteParameters, router: &R,
 		first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES,
-		node_signer: &NS, best_block_height: u32, send_payment_along_path: F
+		node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F,


As @jkczyz pointed out on #1916, there's no need to have a reference to a deref. Maybe we want to fix all occurrences of this at once, though

I think we do have to, or, when I tried to remove it, I got complaints about the deref being unsized. When you do &*<Deref<Target=Logger>> it first has to deref into something Sized, which fails, and then we take a ref again.

Ah, I was confused. I think you should be able to have the function still take a Deref but not use a reference. We do that when passing Deref parameters from ChainMonitor to ChannelMonitor.

I think that would mean we have to add a Clone bound and do a clone, which for an Arc isn't free, sadly.

Hmm... was thinking some thing like this but could be missing something:

rust-lightning/lightning/src/chain/chainmonitor.rs

Line 567 in c59150a

monitor_state.monitor.transaction_unconfirmed(txid, &*self.broadcaster, &*self.fee_estimator, &*self.logger);

Yea, okay, I honestly have no idea. I don't see why that works but this doesn't, but I also can't get it to work 🤷

lightning/src/ln/outbound_payment.rs

valentinewallace · 2023-01-30T20:47:19Z

lightning/src/ln/outbound_payment.rs

-							final_cltv_expiry_delta: max_unsent_cltv_delta,
+							final_cltv_expiry_delta:
+								if let Some(delta) = payment_params.final_cltv_expiry_delta { delta }
+								else { max_unsent_cltv_delta },


Is this supposed to be unreachable like the other place we set final_cltv_expiry_delta? I think we might want the min_unsent_cltv_delta

No, because these are the paymentpatams that the user gave us, which could have None, though they're supposed to set it. The debug assert on the retry pipeline we can do because we update the paymentparams' cltv-delta if the user didn't set one when we fail.

valentinewallace · 2023-01-30T21:05:50Z

lightning/src/ln/outbound_payment.rs

+	fn is_retryable_now(&self, pending_paths: usize) -> bool {
+		match self {
+			PendingOutboundPayment::Retryable { retry_strategy: None, .. } => {
+				// We're handling retries manually, we can always retry.


I thought we were planning to drop support for manual retries. Is there a use case for them? It's a bit confusing that fail_htlc checks payment.get().auto_retries_remaining_now() > 0 instead of is_retryable_now()

Hmm, good question, I'm okay with removing it, I think, I was just trying to keep backwards compat here. Do you want me to remove it here?

Discussed offline, it seems like we should remove support for manual retries pre-release. Since IIUC there's no reason to keep it in this PR, could we remove it, since it adds a bit of review burden?

I think the biggest hurdle is that it's pretty hard to remove in a sensible way while the InvoicePayer is still in-tree. I could remove it in this PR as well, but I understood you wanted to do that later?

Oh, I thought we could remove the bits from this PR without touching InvoicePayer, let's leave it then

Well to remove it we'd have to break InvoicePayer's ability to retry, I think :/

I thought we didn't care about that in between releases since we're removing InvoicePayer anyway, but maybe that'd be a bridge too far. In any case, seems like too small of a complexity diff to be worth too much debate

Maybe it'd break a bunch of tests and other unpleasantness, though.

Right, it'd break ~all the tests.

TheBlueMatt · 2023-01-31T21:59:01Z

Reverted the change of the Retry to being per-retry (not per-path) again but updated the docs which were missing update in 1916. This further simplified the retry checking stuff here a bit.

lightning/src/ln/outbound_payment.rs

valentinewallace · 2023-01-31T23:33:31Z

Github said no more comments for me, but I think the last commit could be its own PR. Otherwise this pretty much LGTM

jkczyz

High-level ACK. Deleted most of my comments as they were addressed in the recent fixups.

lightning/src/ln/outbound_payment.rs

jkczyz · 2023-02-01T00:12:28Z

lightning/src/ln/outbound_payment.rs

-	/// MPP path/HTLC, it will only count as a single retry.
+	/// This is the number of retried sends done. Each send may be multiple HTLCs along multiple
+	/// paths if the router decides to split up a send, and may retry multiple failed HTLCs if they
+	/// failed around the same time and were retried with a single [`Route`].


Not sure I understand why "and were retried with a single [Route]" is included here.

The act of finding the route is the one single thing here - we're retrying multiple HTLCs via multiple HTLCs, the only common thing is we called get_route once. I rephrased it to mention find_route, but let me know if you think I should just drop it. I only like it because otherwise it seems like maybe we're just counting retries based on some arbitrary "did they happen near each other in time" criteria, instead of discrete actions.

valentinewallace · 2023-02-01T16:53:03Z

Meant to say, ACK after squash

jkczyz · 2023-02-01T17:03:35Z

Yeah, feel free to squash

`PaymentParams` is all about the parameters for a payment, i.e. the parameters which are static across all the paths of a paymet. `RouteParameters` is about the information specific to a given `Route` (i.e. a set of paths, among multiple potential sets of paths for a payment). The CLTV delta thus doesn't belong in `RouterParameters` but instead in `PaymentParameters`. Worse, because `RouteParameters` is built from the information in the last hops of a `Route`, when we deliberately inflate the CLTV delta in path-finding, retries of the payment will have the final CLTV delta double-inflated as it inflates starting from the final CLTV delta used in the last attempt. By moving the CLTV delta to `PaymentParameters` we avoid this issue, leaving only the sought amount in the `RouteParameters`.

valentinewallace · 2023-02-01T20:36:59Z

check_commits is failing

`PaymentParams` is all about the parameters for a payment, i.e. the parameters which are static across all the paths of a paymet. `RouteParameters` is about the information specific to a given `Route` (i.e. a set of paths, among multiple potential sets of paths for a payment). The CLTV delta thus doesn't belong in `RouterParameters` but instead in `PaymentParameters`. Worse, because `RouteParameters` is built from the information in the last hops of a `Route`, when we deliberately inflate the CLTV delta in path-finding, retries of the payment will have the final CLTV delta double-inflated as it inflates starting from the final CLTV delta used in the last attempt. When we calculate the `final_cltv_expiry_delta` to put in the `RouteParameters` returned via events after a payment failure, we should re-use the new one in the `PaymentParameters`, rather than the one that was in the route itself.

`TestRouter` allows us to simply select the `Route` that will be returned in the next `find_route` call, but it does so without any checking of what was *requested* for the call. This makes it a somewhat dubious test utility as it very helpfully ensures we ignore errors in the routes we're looking for. Instead, we require users of `TestRouter` pass a `RouteParameters` to `expect_find_route`, which we compare against the requested parameters passed to `find_route`.

The documentation for `Retry` is very clear that it counts the number of failed paths, not discrete retries. When we added retries internally in `ChannelManager`, we switched to counting the number if discrete retries, even if multiple paths failed and were replace with multiple MPP HTLCs. Because we are now rewriting retries, we take this opportunity to reduce the places where retries are analyzed, specifically a good chunk of code is removed from `pay_internal`. Because we now retry multiple failed paths with one single retry, we keep the new behavior, updating the docs on `Retry` to describe the new behavior.

When we landed the initial in-`ChannelManager` payment retries, we stored the `RouteParameters` in the payment info, and then re-use it as-is for new payments. `RouteParameters` is intended to store the information specific to the *route*, `PaymentParameters` exists to store information specific to a payment. Worse, because we don't recalculate the amount stored in the `RouteParameters` before fetching a new route with it, we end up attempting to retry the full payment amount, rather than only the failed part. This issue brought to you by having redundant data in datastructures, part 5,001.

This should marginally reduce compile times for the tests by reducing the total volume of code across the tests in the lightning crate.

TheBlueMatt · 2023-02-01T21:16:53Z

Oops, sorry, bad doc link,

$ git diff-tree -U1 face1912 07656006
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index baae29d1c..e9b7fb949 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -249,4 +249,4 @@ pub(crate) enum HTLCSource {
 		/// backwards) compatibility reasons, but prefer to use the data in the
-		/// [`outbound_payment`] module, which stores per-payment data once instead of in each
-		/// HTLC.
+		/// [`super::outbound_payment`] module, which stores per-payment data once instead of in
+		/// each HTLC.
 		payment_params: Option<PaymentParameters>,

TheBlueMatt added this to the 0.0.114 milestone Jan 28, 2023

TheBlueMatt force-pushed the 2023-01-1916-followups branch 2 times, most recently from deef28a to b2517eb Compare January 30, 2023 18:20

valentinewallace reviewed Jan 30, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-01-1916-followups branch 2 times, most recently from cb45f1e to 41f288a Compare January 31, 2023 21:58

valentinewallace reviewed Jan 31, 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 Show resolved Hide resolved

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

jkczyz reviewed Feb 1, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-01-1916-followups branch from 41f288a to 172241a Compare February 1, 2023 03:30

valentinewallace previously approved these changes Feb 1, 2023

View reviewed changes

valentinewallace mentioned this pull request Feb 1, 2023

Support auto-retrying keysend payments in ChannelManager #2002

Merged

TheBlueMatt dismissed valentinewallace’s stale review via face191 February 1, 2023 17:51

TheBlueMatt force-pushed the 2023-01-1916-followups branch from 172241a to face191 Compare February 1, 2023 17:51

TheBlueMatt added 6 commits February 1, 2023 21:16

Log more information when retrying a payment attempt fails

ddde63e

Reduce the code in the commitment_signed_dance macro

0765600

This should marginally reduce compile times for the tests by reducing the total volume of code across the tests in the lightning crate.

TheBlueMatt force-pushed the 2023-01-1916-followups branch from face191 to 0765600 Compare February 1, 2023 21:16

valentinewallace approved these changes Feb 2, 2023

View reviewed changes

jkczyz approved these changes Feb 2, 2023

View reviewed changes

TheBlueMatt merged commit 2a72f4f into lightningdevkit:main Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1916 Followups Part 1 #1994

1916 Followups Part 1 #1994

TheBlueMatt commented Jan 28, 2023

codecov-commenter commented Jan 28, 2023 •

edited

Loading

valentinewallace Jan 30, 2023

TheBlueMatt Jan 30, 2023

valentinewallace Jan 30, 2023

TheBlueMatt Jan 30, 2023

jkczyz Jan 31, 2023 •

edited

Loading

TheBlueMatt Jan 31, 2023

jkczyz Jan 31, 2023

TheBlueMatt Jan 31, 2023

valentinewallace Jan 30, 2023

TheBlueMatt Jan 30, 2023

valentinewallace Jan 30, 2023

TheBlueMatt Jan 30, 2023

valentinewallace Jan 31, 2023

TheBlueMatt Jan 31, 2023

valentinewallace Jan 31, 2023

TheBlueMatt Jan 31, 2023

valentinewallace Jan 31, 2023

valentinewallace Jan 31, 2023

TheBlueMatt Jan 31, 2023

TheBlueMatt commented Jan 31, 2023

valentinewallace commented Jan 31, 2023

jkczyz left a comment

jkczyz Feb 1, 2023

TheBlueMatt Feb 1, 2023 •

edited

Loading

valentinewallace commented Feb 1, 2023

jkczyz commented Feb 1, 2023

valentinewallace commented Feb 1, 2023

TheBlueMatt commented Feb 1, 2023

1916 Followups Part 1 #1994

1916 Followups Part 1 #1994

Conversation

TheBlueMatt commented Jan 28, 2023

codecov-commenter commented Jan 28, 2023 • edited Loading

Codecov Report

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

jkczyz Jan 31, 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

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

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 Jan 31, 2023

valentinewallace commented Jan 31, 2023

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

valentinewallace commented Feb 1, 2023

jkczyz commented Feb 1, 2023

valentinewallace commented Feb 1, 2023

TheBlueMatt commented Feb 1, 2023

codecov-commenter commented Jan 28, 2023 •

edited

Loading

jkczyz Jan 31, 2023 •

edited

Loading

TheBlueMatt Feb 1, 2023 •

edited

Loading