diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b6f1c032f..0f0260723 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -14863,6 +14863,7 @@ mod tests { }, _ => panic!("unexpected error") } + assert!(nodes[0].node.list_recent_payments().is_empty()); } #[test] diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 2a0550a54..b027f64f1 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -6483,6 +6483,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. diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 070a09fde..9c28e43ff 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -811,7 +811,7 @@ impl OutboundPayments { { let onion_session_privs = self.add_new_pending_payment(payment_hash, recipient_onion.clone(), payment_id, None, route, None, None, entropy_source, best_block_height)?; self.pay_route_internal(route, payment_hash, &recipient_onion, None, None, payment_id, None, - onion_session_privs, node_signer, best_block_height, &send_payment_along_path) + &onion_session_privs, node_signer, best_block_height, &send_payment_along_path) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } @@ -991,7 +991,7 @@ impl OutboundPayments { let result = self.pay_route_internal( &route, payment_hash, &recipient_onion, keysend_preimage, invoice_request, payment_id, - Some(route_params.final_value_msat), onion_session_privs, node_signer, best_block_height, + Some(route_params.final_value_msat), &onion_session_privs, node_signer, best_block_height, &send_payment_along_path ); log_info!( @@ -1000,9 +1000,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(()) @@ -1276,12 +1276,16 @@ impl OutboundPayments { })?; let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, - keysend_preimage, None, payment_id, None, onion_session_privs, node_signer, + keysend_preimage, None, payment_id, None, &onion_session_privs, node_signer, best_block_height, &send_payment_along_path); 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(()) } @@ -1433,19 +1437,25 @@ impl OutboundPayments { } }; let res = self.pay_route_internal(&route, payment_hash, &recipient_onion, keysend_preimage, - invoice_request.as_ref(), payment_id, Some(total_msat), onion_session_privs, node_signer, + invoice_request.as_ref(), payment_id, Some(total_msat), &onion_session_privs, node_signer, 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( &self, err: PaymentSendFailure, payment_id: PaymentId, payment_hash: PaymentHash, route: Route, - mut route_params: RouteParameters, router: &R, first_hops: Vec, - inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32, logger: &L, - pending_events: &Mutex)>>, send_payment_along_path: &SP, + mut route_params: RouteParameters, onion_session_privs: Vec<[u8; 32]>, router: &R, + first_hops: Vec, inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, + best_block_height: u32, logger: &L, + pending_events: &Mutex)>>, + send_payment_along_path: &SP, ) where R::Target: Router, @@ -1457,10 +1467,24 @@ impl OutboundPayments { { match err { PaymentSendFailure::AllFailedResendSafe(errs) => { + self.remove_session_privs(payment_id, route.paths.iter().zip(onion_session_privs.iter())); 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); 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, .. } => { + debug_assert_eq!(results.len(), route.paths.len()); + debug_assert_eq!(results.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 + // considered "in flight" and we shouldn't remove it from the + // PendingOutboundPayment set. + Ok(_) | Err(APIError::MonitorUpdateInProgress) => None, + _ => Some((path, session_priv)) + } + }); + 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 @@ -1474,11 +1498,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.paths.iter().zip(onion_session_privs.iter())); 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.paths.iter().zip(onion_session_privs.iter())); self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events); }, PaymentSendFailure::DuplicatePayment => debug_assert!(false), // unreachable @@ -1518,6 +1544,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<'a, I: Iterator>( + &self, payment_id: PaymentId, path_session_priv: I + ) { + if let Some(payment) = self.pending_outbound_payments.lock().unwrap().get_mut(&payment_id) { + for (path, session_priv_bytes) in path_session_priv { + 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( &self, path: Path, probing_cookie_secret: [u8; 32], entropy_source: &ES, node_signer: &NS, best_block_height: u32, send_payment_along_path: F @@ -1549,7 +1590,7 @@ impl OutboundPayments { let recipient_onion_fields = RecipientOnionFields::spontaneous_empty(); match self.pay_route_internal(&route, payment_hash, &recipient_onion_fields, - None, None, payment_id, None, onion_session_privs, node_signer, best_block_height, + None, None, payment_id, None, &onion_session_privs, node_signer, best_block_height, &send_payment_along_path ) { Ok(()) => Ok((payment_hash, payment_id)), @@ -1740,7 +1781,7 @@ impl OutboundPayments { fn pay_route_internal( &self, route: &Route, payment_hash: PaymentHash, recipient_onion: &RecipientOnionFields, keysend_preimage: Option, invoice_request: Option<&InvoiceRequest>, - payment_id: PaymentId, recv_value_msat: Option, onion_session_privs: Vec<[u8; 32]>, + payment_id: PaymentId, recv_value_msat: Option, onion_session_privs: &Vec<[u8; 32]>, node_signer: &NS, best_block_height: u32, send_payment_along_path: &F ) -> Result<(), PaymentSendFailure> where @@ -1791,30 +1832,12 @@ 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()) { - let mut path_res = send_payment_along_path(SendAlongPathArgs { + for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) { + let 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, - session_priv_bytes + session_priv_bytes: *session_priv_bytes }); - match path_res { - Ok(_) => {}, - Err(APIError::MonitorUpdateInProgress) => { - // While a MonitorUpdateInProgress is an Err(_), the payment is still - // considered "in flight" and we shouldn't remove it from the - // PendingOutboundPayment set. - }, - Err(_) => { - let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); - if let Some(payment) = pending_outbounds.get_mut(&payment_id) { - 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"); - path_res = Err(APIError::APIMisuseError { err: "Internal error: payment disappeared during processing. Please report this bug!".to_owned() }); - } - } - } results.push(path_res); } let mut has_ok = false; @@ -1879,7 +1902,7 @@ impl OutboundPayments { F: Fn(SendAlongPathArgs) -> Result<(), APIError>, { self.pay_route_internal(route, payment_hash, &recipient_onion, - keysend_preimage, None, payment_id, recv_value_msat, onion_session_privs, + keysend_preimage, None, payment_id, recv_value_msat, &onion_session_privs, node_signer, best_block_height, &send_payment_along_path) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } @@ -1887,9 +1910,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 { .. } => {} } } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 0c9c5d0e9..0b06a18ea 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/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()); +} diff --git a/pending_changelog/3531-buggy-router-leak.txt b/pending_changelog/3531-buggy-router-leak.txt new file mode 100644 index 000000000..72714aa8a --- /dev/null +++ b/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).