Fix outbound payments memory leak on buggy router

[valentinewallace]

Prior to this patch, if we attempted to send a payment or probe to a buggy route, we would error but continue storing the pending outbound payment forever. Attempts to retry would result in a “duplicate payment” error.

In the case of ChannelManager::send_payment, we would also fail to generate a PaymentFailed event, even if the user manually called abandon_payment.

This bug is unlikely to have ever been hit in the wild as most users use LDK’s router. Discovered in the course of adding a new send_to_route API.

Now, we’ll properly generate events and remove the outbound from storage.

[tnull]

LGTM

but continue storing the pending outbound payment forever.

Hmm, I wonder if there is any way to detect these cases in retrospect? I.e., could we prune them in remove_stale_payments so that they are dropped after upgrade and not linger forever in the storage of users that happened to hit the bug?

[TheBlueMatt]

Good catch!

[valentinewallace]

Hmm, I wonder if there is any way to detect these cases in retrospect? I.e., could we prune them in remove_stale_payments so that they are dropped after upgrade and not linger forever in the storage of users that happened to hit the bug?

@tnull I did consider this, it didn't seem worth it at the time because it means introducing extra code for a weird/unlikely case 🤔 One option would be to remove long-expired Retryables based on their PaymentParameters, which is an optional field that was added in 0.0.114. Block height is another option. Let me know if you think this is worth it.

[tnull]

@tnull I did consider this, it didn't seem worth it at the time because it means introducing extra code for a weird/unlikely case 🤔 One option would be to remove long-expired Retryables based on their PaymentParameters, which is an optional field that was added in 0.0.114. Block height is another option. Let me know if you think this is worth it.

If we're positive this likely really never happened in production, it's probably not worth it I guess.

[TheBlueMatt]

It looks like a user could have tried to pay an invoice with the MPP feature but without a payment secret and we could have hit this leak? I'm not sure that means we need to run the expiry backfill, though...if we do it probably means we'll want to run it on deserialization against the actual pending HTLCs-in-flight list, rather than just looking at the outbound payments in isolation.

[valentinewallace]

Definitely fine by me to fix it if y'all prefer, though I'm preeetty hesitant to add more complexity to ChannelManager deserialization. I see why doing it that way is more robust but I guess I'm not really following why doing it in remove_stale_payments isn't sufficient.

[valentinewallace]

Squashed in a macro -> function and linting fix:

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index d7e6b1869..8517d599c 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -1458,19 +1458,6 @@ impl OutboundPayments {
                IH: Fn() -> InFlightHtlcs,
                SP: Fn(SendAlongPathArgs) -> Result<(), APIError>,
        {
-               macro_rules! remove_session_privs {
-                       () => {
-                               if let Some(payment) = self.pending_outbound_payments.lock().unwrap().get_mut(&payment_id) {
-                                       for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.into_iter()) {
-                                               let removed = payment.remove(&session_priv_bytes, Some(path));
-                                               debug_assert!(removed, "This can't happen as the payment has an entry for this path added by callers");
-                                       }
-                               } else {
-                                       debug_assert!(false, "This can't happen as the payment was added by callers");
-                               }
-                       }
-               }
-
                match err {
                        PaymentSendFailure::AllFailedResendSafe(errs) => {
                                Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, errs.into_iter().map(|e| Err(e)), logger, pending_events);
@@ -1490,13 +1477,13 @@ impl OutboundPayments {
                        },
                        PaymentSendFailure::PathParameterError(results) => {
                                log_error!(logger, "Failed to send to route due to parameter error in a single path. Your router is buggy");
-                               remove_session_privs!();
+                               self.remove_session_privs(payment_id, &route, onion_session_privs);
                                Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, results.into_iter(), logger, pending_events);
                                self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events);
                        },
                        PaymentSendFailure::ParameterError(e) => {
                                log_error!(logger, "Failed to send to route due to parameter error: {:?}. Your router is buggy", e);
-                               remove_session_privs!();
+                               self.remove_session_privs(payment_id, &route, onion_session_privs);
                                self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events);
                        },
                        PaymentSendFailure::DuplicatePayment => debug_assert!(false), // unreachable
@@ -1536,6 +1523,21 @@ impl OutboundPayments {
                }
        }

+       // If a payment fails after adding the pending payment but before any HTLCs are locked into
+       // channels, we need to clear the session_privs in order for abandoning the payment to succeed.
+       fn remove_session_privs(
+               &self, payment_id: PaymentId, route: &Route, onion_session_privs: Vec<[u8; 32]>
+       ) {
+               if let Some(payment) = self.pending_outbound_payments.lock().unwrap().get_mut(&payment_id) {
+                       for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.into_iter()) {
+                               let removed = payment.remove(&session_priv_bytes, Some(path));
+                               debug_assert!(removed, "This can't happen as the payment has an entry for this path added by callers");
+                       }
+               } else {
+                       debug_assert!(false, "This can't happen as the payment was added by callers");
+               }
+       }
+
        pub(super) fn send_probe<ES: Deref, NS: Deref, F>(
                &self, path: Path, probing_cookie_secret: [u8; 32], entropy_source: &ES, node_signer: &NS,
                best_block_height: u32, send_payment_along_path: F
@@ -1809,7 +1811,7 @@ impl OutboundPayments {
                let cur_height = best_block_height + 1;
                let mut results = Vec::new();
                debug_assert_eq!(route.paths.len(), onion_session_privs.len());
-               for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.into_iter()) {
+               for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
                        let mut path_res = send_payment_along_path(SendAlongPathArgs {
                                path: &path, payment_hash: &payment_hash, recipient_onion, total_value,
                                cur_height, payment_id, keysend_preimage: &keysend_preimage, invoice_request,

[valentinewallace]

It looks like unfortunately all tests pass with this diff:

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 68d4125a3..430fb166c 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -1454,7 +1454,8 @@ impl OutboundPayments {
                                                        // While a MonitorUpdateInProgress is an Err(_), the payment is still
                                                        // considered "in flight" and we shouldn't remove it from the
                                                        // PendingOutboundPayment set.
-                                                       Ok(_) | Err(APIError::MonitorUpdateInProgress) => None,
+                                                       Ok(_) => None,
                                                        _ => Some((path, session_priv))
                                                }
                                        });

Even in the follow-up #3534 when rebased on this PR. Seems pre-existing though.

[TheBlueMatt]

LGTM.

[valentinewallace]

Added a debug_assert and renamed a variable:

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 98efd7b5d..8de47c4ae 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -1465,7 +1465,8 @@ impl OutboundPayments {
                                self.find_route_and_send_payment(payment_hash, payment_id, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, send_payment_along_path);
                        },
                        PaymentSendFailure::PartialFailure { failed_paths_retry: Some(mut retry), results, .. } => {
-                               let filtered = results.iter().zip(route.paths.iter().zip(onion_session_privs.iter()))
+                               debug_assert_eq!(route.paths.len(), onion_session_privs.len());
+                               let failed_paths = results.iter().zip(route.paths.iter().zip(onion_session_privs.iter()))
                                        .filter_map(|(path_res, (path, session_priv))| {
                                                match path_res {
                                                        // While a MonitorUpdateInProgress is an Err(_), the payment is still
@@ -1475,7 +1476,7 @@ impl OutboundPayments {
                                                        _ => Some((path, session_priv))
                                                }
                                        });
-                               self.remove_session_privs(payment_id, filtered);
+                               self.remove_session_privs(payment_id, failed_paths);
                                Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut retry, route.paths, results.into_iter(), logger, pending_events);
                                // Some paths were sent, even if we failed to send the full MPP value our recipient may
                                // misbehave and claim the funds, at which point we have to consider the payment sent, so

[valentinewallace]

Ah wait, need one more debug_assert.

Edit: done

[TheBlueMatt]

Backported in #3567