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

@@ -2421,6 +2421,9 @@ where
 	/// See `PendingOutboundPayment` documentation for more info.
 	///
 	/// See `ChannelManager` struct-level documentation for lock order requirements.
+	#[cfg(test)]
+	pub(super) pending_outbound_payments: OutboundPayments,
+	#[cfg(not(test))]
 	pending_outbound_payments: OutboundPayments,
 
 	/// SCID/SCID Alias -> forward infos. Key of 0 means payments received.
@@ -14783,6 +14786,7 @@ mod tests {
 			},
 			_ => panic!("unexpected error")
 		}
+		assert!(nodes[0].node.pending_outbound_payments.pending_outbound_payments.lock().unwrap().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.pending_outbound_payments.pending_outbound_payments.lock().unwrap().is_empty());
 }
 
 // BOLT 2 Requirements for the Sender when constructing and sending an update_add_htlc message.

lightning/src/ln/outbound_payment.rs

@@ -1740,12 +1740,26 @@ impl OutboundPayments {
 		NS::Target: NodeSigner,
 		F: 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");
+				}
+			}
+		}
+
 		if route.paths.len() < 1 {
 			return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over".to_owned()}));
 		}
 		if recipient_onion.payment_secret.is_none() && route.paths.len() > 1
 			&& !route.paths.iter().any(|p| p.blinded_tail.is_some())
 		{
+			remove_session_privs!();
 			return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError{err: "Payment secret is required for multi-path payments".to_owned()}));
 		}
 		let mut total_value = 0;
@@ -1775,6 +1789,7 @@ impl OutboundPayments {
 			path_errs.push(Ok(()));
 		}
 		if path_errs.iter().any(|e| e.is_err()) {
+			remove_session_privs!();
 			return Err(PaymentSendFailure::PathParameterError(path_errs));
 		}
 		if let Some(amt_msat) = recv_value_msat {
@@ -1880,9 +1895,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");
+			},
+			_ => {}
 		}
 	}
 

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.pending_outbound_payments.pending_outbound_payments.lock().unwrap().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.pending_outbound_payments.pending_outbound_payments.lock().unwrap().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.pending_outbound_payments.pending_outbound_payments.lock().unwrap().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).