Fix outbound payments memory leak on buggy router

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.

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -14783,6 +14783,7 @@ mod tests {
 			},
 			_ => panic!("unexpected error")
 		}
+		assert!(nodes[0].node.list_recent_payments().is_empty());
 	}
 
 	#[test]

lightning/src/ln/functional_tests.rs

@@ -6235,6 +6235,7 @@ fn test_payment_route_reaching_same_channel_twice() {
 		RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0)
 	), false, APIError::InvalidRoute { ref err },
 	assert_eq!(err, &"Path went through the same channel twice"));
+	assert!(nodes[0].node.list_recent_payments().is_empty());
 }
 
 // BOLT 2 Requirements for the Sender when constructing and sending an update_add_htlc message.

lightning/src/ln/outbound_payment.rs

@@ -992,9 +992,9 @@ impl OutboundPayments {
 		);
 		if let Err(e) = result {
 			self.handle_pay_route_err(
-				e, payment_id, payment_hash, route, route_params, router, first_hops,
-				&inflight_htlcs, entropy_source, node_signer, best_block_height, logger,
-				pending_events, &send_payment_along_path
+				e, payment_id, payment_hash, route, route_params, onion_session_privs, router, first_hops,
+				&inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events,
+				&send_payment_along_path
 			);
 		}
 		Ok(())
@@ -1274,7 +1274,11 @@ impl OutboundPayments {
 		log_info!(logger, "Sending payment with id {} and hash {} returned {:?}",
 			payment_id, payment_hash, res);
 		if let Err(e) = res {
-			self.handle_pay_route_err(e, payment_id, payment_hash, route, route_params, router, first_hops, &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, &send_payment_along_path);
+			self.handle_pay_route_err(
+				e, payment_id, payment_hash, route, route_params, onion_session_privs, router, first_hops,
+				&inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events,
+				&send_payment_along_path
+			);
 		}
 		Ok(())
 	}
@@ -1430,15 +1434,21 @@ impl OutboundPayments {
 			best_block_height, &send_payment_along_path);
 		log_info!(logger, "Result retrying payment id {}: {:?}", &payment_id, res);
 		if let Err(e) = res {
-			self.handle_pay_route_err(e, payment_id, payment_hash, route, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, send_payment_along_path);
+			self.handle_pay_route_err(
+				e, payment_id, payment_hash, route, route_params, onion_session_privs, router, first_hops,
+				inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events,
+				send_payment_along_path
+			);
 		}
 	}
 
 	fn handle_pay_route_err<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>(
 		&self, err: PaymentSendFailure, payment_id: PaymentId, payment_hash: PaymentHash, route: Route,
-		mut route_params: RouteParameters, router: &R, first_hops: Vec<ChannelDetails>,
-		inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32, logger: &L,
-		pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>, send_payment_along_path: &SP,
+		mut route_params: RouteParameters, onion_session_privs: Vec<[u8; 32]>, router: &R,
+		first_hops: Vec<ChannelDetails>, inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS,
+		best_block_height: u32, logger: &L,
+		pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>,
+		send_payment_along_path: &SP,
 	)
 	where
 		R::Target: Router,
@@ -1467,11 +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");
+				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);
+				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
@@ -1511,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
@@ -1784,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,
@@ -1880,9 +1907,15 @@ impl OutboundPayments {
 	// If we failed to send any paths, remove the new PaymentId from the `pending_outbound_payments`
 	// map as the payment is free to be resent.
 	fn remove_outbound_if_all_failed(&self, payment_id: PaymentId, err: &PaymentSendFailure) {
-		if let &PaymentSendFailure::AllFailedResendSafe(_) = err {
-			let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some();
-			debug_assert!(removed, "We should always have a pending payment to remove here");
+		match err {
+			PaymentSendFailure::AllFailedResendSafe(_)
+				| PaymentSendFailure::ParameterError(_)
+				| PaymentSendFailure::PathParameterError(_) =>
+			{
+				let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some();
+				debug_assert!(removed, "We should always have a pending payment to remove here");
+			},
+			PaymentSendFailure::DuplicatePayment | PaymentSendFailure::PartialFailure { .. }  => {}
 		}
 	}
 

lightning/src/ln/payment_tests.rs

@@ -24,7 +24,7 @@ use crate::types::payment::{PaymentHash, PaymentSecret, PaymentPreimage};
 use crate::ln::chan_utils;
 use crate::ln::msgs::ChannelMessageHandler;
 use crate::ln::onion_utils;
-use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, Retry, RetryableSendFailure};
+use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, ProbeSendFailure, Retry, RetryableSendFailure};
 use crate::routing::gossip::{EffectiveCapacity, RoutingFees};
 use crate::routing::router::{get_route, Path, PaymentParameters, Route, Router, RouteHint, RouteHintHop, RouteHop, RouteParameters};
 use crate::routing::scoring::ChannelUsage;
@@ -1249,6 +1249,7 @@ fn sent_probe_is_probe_of_sending_node() {
 	// First check we refuse to build a single-hop probe
 	let (route, _, _, _) = get_route_and_payment_hash!(&nodes[0], nodes[1], 100_000);
 	assert!(nodes[0].node.send_probe(route.paths[0].clone()).is_err());
+	assert!(nodes[0].node.list_recent_payments().is_empty());
 
 	// Then build an actual two-hop probing path
 	let (route, _, _, _) = get_route_and_payment_hash!(&nodes[0], nodes[2], 100_000);
@@ -4375,3 +4376,77 @@ fn test_non_strict_forwarding() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	expect_payment_failed_conditions_event(events, payment_hash, false, PaymentFailedConditions::new().blamed_scid(routed_scid));
 }
+
+#[test]
+fn remove_pending_outbounds_on_buggy_router() {
+	// Ensure that if a payment errors due to a bogus route, we'll abandon the payment and remove the
+	// pending outbound from storage.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	create_announced_chan_between_nodes(&nodes, 0, 1);
+
+	let amt_msat = 10_000;
+	let payment_id = PaymentId([42; 32]);
+	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), 0)
+		.with_bolt11_features(nodes[1].node.bolt11_invoice_features()).unwrap();
+	let (mut route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], payment_params, amt_msat);
+
+	// Extend the path by itself, essentially simulating route going through same channel twice
+	let cloned_hops = route.paths[0].hops.clone();
+	route.paths[0].hops.extend_from_slice(&cloned_hops);
+	let route_params = route.route_params.clone().unwrap();
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
+
+	nodes[0].node.send_payment(
+		payment_hash, RecipientOnionFields::secret_only(payment_secret), payment_id, route_params,
+		Retry::Attempts(1) // Even though another attempt is allowed, the payment should fail
+	).unwrap();
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 2);
+	match &events[0] {
+		Event::PaymentPathFailed { failure, payment_failed_permanently, .. } => {
+			assert_eq!(failure, &PathFailure::InitialSend {
+				err: APIError::InvalidRoute { err: "Path went through the same channel twice".to_string() }
+			});
+			assert!(!payment_failed_permanently);
+		},
+		_ => panic!()
+	}
+	match events[1] {
+		Event::PaymentFailed { reason, .. } => {
+			assert_eq!(reason.unwrap(), PaymentFailureReason::UnexpectedError);
+		},
+		_ => panic!()
+	}
+	assert!(nodes[0].node.list_recent_payments().is_empty());
+}
+
+#[test]
+fn remove_pending_outbound_probe_on_buggy_path() {
+	// Ensure that if a probe errors due to a bogus route, we'll return an error and remove the
+	// pending outbound from storage.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	create_announced_chan_between_nodes(&nodes, 0, 1);
+
+	let amt_msat = 10_000;
+	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), 0)
+		.with_bolt11_features(nodes[1].node.bolt11_invoice_features()).unwrap();
+	let (mut route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], payment_params, amt_msat);
+
+	// Extend the path by itself, essentially simulating route going through same channel twice
+	let cloned_hops = route.paths[0].hops.clone();
+	route.paths[0].hops.extend_from_slice(&cloned_hops);
+
+	assert_eq!(
+		nodes[0].node.send_probe(route.paths.pop().unwrap()).unwrap_err(),
+		ProbeSendFailure::ParameterError(
+			APIError::InvalidRoute { err: "Path went through the same channel twice".to_string() }
+		)
+	);
+	assert!(nodes[0].node.list_recent_payments().is_empty());
+}

pending_changelog/3531-buggy-router-leak.txt

@@ -0,0 +1,4 @@
+## Bug Fixes
+
+* Fixed a rare case where a custom router returning a buggy route could result in holding onto a
+	pending payment forever and in some cases failing to generate a PaymentFailed event (#3531).