From aa55366b376b1f064c3975cbe46a6b78e1e1c8eb Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 10 Jul 2024 16:03:31 -0500 Subject: [PATCH 1/4] Add Bolt12PaymentError::SendingFailed variant Instead of returning Ok when path finding fails, allow returning a RetryableSendFailure from send_payment_for_bolt12_invoice. Follow up commits will return such failures. --- lightning/src/ln/max_payment_path_len_tests.rs | 2 +- lightning/src/ln/outbound_payment.rs | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/max_payment_path_len_tests.rs b/lightning/src/ln/max_payment_path_len_tests.rs index 096bcf9633c..fd027ea62b6 100644 --- a/lightning/src/ln/max_payment_path_len_tests.rs +++ b/lightning/src/ln/max_payment_path_len_tests.rs @@ -388,7 +388,7 @@ fn bolt12_invoice_too_large_blinded_paths() { let invoice_om = nodes[1].onion_messenger.next_onion_message_for_peer(nodes[0].node.get_our_node_id()).unwrap(); nodes[0].onion_messenger.handle_onion_message(&nodes[1].node.get_our_node_id(), &invoice_om); // TODO: assert on the invoice error once we support replying to invoice OMs with failure info - nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Failed paying invoice: OnionPacketSizeExceeded", 1); + nodes[0].logger.assert_log_contains("lightning::ln::channelmanager", "Failed paying invoice: SendingFailed(OnionPacketSizeExceeded)", 1); let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 443a7b2c3a2..42c100cde28 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -510,11 +510,8 @@ pub enum Bolt12PaymentError { UnexpectedInvoice, /// Payment for an invoice with the corresponding [`PaymentId`] was already initiated. DuplicateInvoice, - /// The [`BlindedPath`]s provided are too large and caused us to exceed the maximum onion hop data - /// size of 1300 bytes. - /// - /// [`BlindedPath`]: crate::blinded_path::BlindedPath - OnionPacketSizeExceeded, + /// The invoice was valid for the corresponding [`PaymentId`], but sending the payment failed. + SendingFailed(RetryableSendFailure), } /// Indicates that we failed to send a payment probe. Further errors may be surfaced later via @@ -848,7 +845,7 @@ impl OutboundPayments { .map_err(|()| { log_error!(logger, "Can't construct an onion packet without exceeding 1300-byte onion \ hop_data length for payment with id {} and hash {}", payment_id, payment_hash); - Bolt12PaymentError::OnionPacketSizeExceeded + Bolt12PaymentError::SendingFailed(RetryableSendFailure::OnionPacketSizeExceeded) })?; if let Some(max_fee_msat) = max_total_routing_fee_msat { From c8fd681d4f269f0fd3f683de8c5b1f9842a6600a Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 11 Jul 2024 15:18:40 -0500 Subject: [PATCH 2/4] Refactor initial route finding to separate method This will be used later in send_payment_for_bolt12_invoice instead of find_route_and_send_payment as it will allow for returning RetryableSendFailure when path finding fails. --- lightning/src/ln/outbound_payment.rs | 54 +++++++++++++++++++--------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 42c100cde28..2c929952e98 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -925,25 +925,17 @@ impl OutboundPayments { !pmt.is_awaiting_invoice()) } - /// Errors immediately on [`RetryableSendFailure`] error conditions. Otherwise, further errors may - /// be surfaced asynchronously via [`Event::PaymentPathFailed`] and [`Event::PaymentFailed`]. - /// - /// [`Event::PaymentPathFailed`]: crate::events::Event::PaymentPathFailed - /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed - fn send_payment_internal( - &self, payment_id: PaymentId, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, - keysend_preimage: Option, retry_strategy: Retry, 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, - ) -> Result<(), RetryableSendFailure> + fn find_initial_route( + &self, payment_id: PaymentId, payment_hash: PaymentHash, + recipient_onion: &RecipientOnionFields, keysend_preimage: Option, + route_params: &mut RouteParameters, router: &R, first_hops: &Vec, + inflight_htlcs: &IH, node_signer: &NS, best_block_height: u32, logger: &L, + ) -> Result where R::Target: Router, - ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, IH: Fn() -> InFlightHtlcs, - SP: Fn(SendAlongPathArgs) -> Result<(), APIError>, { #[cfg(feature = "std")] { if has_expired(&route_params) { @@ -954,7 +946,7 @@ impl OutboundPayments { } onion_utils::set_max_path_length( - &mut route_params, &recipient_onion, keysend_preimage, best_block_height + route_params, recipient_onion, keysend_preimage, best_block_height ) .map_err(|()| { log_error!(logger, "Can't construct an onion packet without exceeding 1300-byte onion \ @@ -963,7 +955,7 @@ impl OutboundPayments { })?; let mut route = router.find_route_with_id( - &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, + &node_signer.get_node_id(Recipient::Node).unwrap(), route_params, Some(&first_hops.iter().collect::>()), inflight_htlcs(), payment_hash, payment_id, ).map_err(|_| { @@ -972,12 +964,40 @@ impl OutboundPayments { RetryableSendFailure::RouteNotFound })?; - if route.route_params.as_ref() != Some(&route_params) { + if route.route_params.as_ref() != Some(route_params) { debug_assert!(false, "Routers are expected to return a Route which includes the requested RouteParameters"); route.route_params = Some(route_params.clone()); } + Ok(route) + } + + /// Errors immediately on [`RetryableSendFailure`] error conditions. Otherwise, further errors may + /// be surfaced asynchronously via [`Event::PaymentPathFailed`] and [`Event::PaymentFailed`]. + /// + /// [`Event::PaymentPathFailed`]: crate::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed + fn send_payment_internal( + &self, payment_id: PaymentId, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, + keysend_preimage: Option, retry_strategy: Retry, 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, + ) -> Result<(), RetryableSendFailure> + where + R::Target: Router, + ES::Target: EntropySource, + NS::Target: NodeSigner, + L::Target: Logger, + IH: Fn() -> InFlightHtlcs, + SP: Fn(SendAlongPathArgs) -> Result<(), APIError>, + { + let route = self.find_initial_route( + payment_id, payment_hash, &recipient_onion, keysend_preimage, &mut route_params, router, + &first_hops, &inflight_htlcs, node_signer, best_block_height, logger, + )?; + let onion_session_privs = self.add_new_pending_payment(payment_hash, recipient_onion.clone(), payment_id, keysend_preimage, &route, Some(retry_strategy), Some(route_params.payment_params.clone()), entropy_source, best_block_height) From 23a1eee6ece9b6af96aca74a9cadf918a0730b4f Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 11 Jul 2024 17:07:53 -0500 Subject: [PATCH 3/4] Refactor send_payment_for_bolt12_invoice The BOLT11 and BOLT12 outbound payment initiation code differ in that the latter re-uses the retry path (i.e., find_route_and_send_payment). The drawback of this is that Ok is returned even if there is an error finding a route. Refactor send_payment_for_bolt12_invoice such that it re-uses find_initial_route instead so that errors can be returned. --- lightning/src/ln/outbound_payment.rs | 96 ++++++++++++++++++---------- 1 file changed, 64 insertions(+), 32 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 2c929952e98..e894b4e3844 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -800,20 +800,24 @@ impl OutboundPayments { { let payment_hash = invoice.payment_hash(); let max_total_routing_fee_msat; + let retry_strategy; match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { hash_map::Entry::Occupied(entry) => match entry.get() { - PendingOutboundPayment::AwaitingInvoice { retry_strategy, max_total_routing_fee_msat: max_total_fee, .. } => { + PendingOutboundPayment::AwaitingInvoice { + retry_strategy: retry, max_total_routing_fee_msat: max_total_fee, .. + } => { + retry_strategy = Some(*retry); max_total_routing_fee_msat = *max_total_fee; *entry.into_mut() = PendingOutboundPayment::InvoiceReceived { payment_hash, - retry_strategy: *retry_strategy, + retry_strategy: *retry, max_total_routing_fee_msat, }; }, _ => return Err(Bolt12PaymentError::DuplicateInvoice), }, hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice), - }; + } let mut payment_params = PaymentParameters::from_bolt12_invoice(&invoice); @@ -839,25 +843,64 @@ impl OutboundPayments { let mut route_params = RouteParameters::from_payment_params_and_value( payment_params, amount_msat ); - onion_utils::set_max_path_length( - &mut route_params, &RecipientOnionFields::spontaneous_empty(), None, best_block_height - ) - .map_err(|()| { - log_error!(logger, "Can't construct an onion packet without exceeding 1300-byte onion \ - hop_data length for payment with id {} and hash {}", payment_id, payment_hash); - Bolt12PaymentError::SendingFailed(RetryableSendFailure::OnionPacketSizeExceeded) - })?; if let Some(max_fee_msat) = max_total_routing_fee_msat { route_params.max_total_routing_fee_msat = Some(max_fee_msat); } - 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 + let recipient_onion = RecipientOnionFields { + payment_secret: None, + payment_metadata: None, + custom_tlvs: vec![], + }; + let route = match self.find_initial_route( + payment_id, payment_hash, &recipient_onion, None, &mut route_params, router, + &first_hops, &inflight_htlcs, node_signer, best_block_height, logger, + ) { + Ok(route) => route, + Err(e) => { + let reason = match e { + RetryableSendFailure::PaymentExpired => PaymentFailureReason::PaymentExpired, + RetryableSendFailure::RouteNotFound => PaymentFailureReason::RouteNotFound, + RetryableSendFailure::DuplicatePayment => PaymentFailureReason::UnexpectedError, + RetryableSendFailure::OnionPacketSizeExceeded => PaymentFailureReason::UnexpectedError, + }; + self.abandon_payment(payment_id, reason, pending_events); + return Err(Bolt12PaymentError::SendingFailed(e)); + }, + }; + + let payment_params = Some(route_params.payment_params.clone()); + let (retryable_payment, onion_session_privs) = self.create_pending_payment( + payment_hash, recipient_onion.clone(), None, &route, + retry_strategy, payment_params, entropy_source, best_block_height ); + match self.pending_outbound_payments.lock().unwrap().entry(payment_id) { + hash_map::Entry::Occupied(entry) => match entry.get() { + PendingOutboundPayment::InvoiceReceived { .. } => { + *entry.into_mut() = retryable_payment; + }, + _ => return Err(Bolt12PaymentError::DuplicateInvoice), + }, + hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice), + } + let result = self.pay_route_internal( + &route, payment_hash, &recipient_onion, None, payment_id, + Some(route_params.final_value_msat), 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, result + ); + 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 + ); + } Ok(()) } @@ -1134,21 +1177,10 @@ impl OutboundPayments { log_error!(logger, "Payment not yet sent"); return }, - PendingOutboundPayment::InvoiceReceived { payment_hash, retry_strategy, .. } => { - let total_amount = route_params.final_value_msat; - let recipient_onion = RecipientOnionFields { - payment_secret: None, - payment_metadata: None, - custom_tlvs: vec![], - }; - let retry_strategy = Some(*retry_strategy); - let payment_params = Some(route_params.payment_params.clone()); - let (retryable_payment, onion_session_privs) = self.create_pending_payment( - *payment_hash, recipient_onion.clone(), None, &route, - retry_strategy, payment_params, entropy_source, best_block_height - ); - *payment.into_mut() = retryable_payment; - (total_amount, recipient_onion, None, onion_session_privs) + PendingOutboundPayment::InvoiceReceived { .. } => { + log_error!(logger, "Payment already initiating"); + debug_assert!(false); + return }, PendingOutboundPayment::Fulfilled { .. } => { log_error!(logger, "Payment already completed"); @@ -2290,7 +2322,7 @@ mod tests { &&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events, |_| panic!() ), - Ok(()), + Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::PaymentExpired)), ); assert!(!outbound_payments.has_pending_payments()); @@ -2351,7 +2383,7 @@ mod tests { &&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events, |_| panic!() ), - Ok(()), + Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::RouteNotFound)), ); assert!(!outbound_payments.has_pending_payments()); From b697fbe78b952aa1a9c3932b7a22d159ba337669 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 18 Jul 2024 10:24:09 -0500 Subject: [PATCH 4/4] Add debug_assert!(false) for unreachable code --- lightning/src/ln/outbound_payment.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index e894b4e3844..43b636fb7ff 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -1175,6 +1175,7 @@ impl OutboundPayments { }, PendingOutboundPayment::AwaitingInvoice { .. } => { log_error!(logger, "Payment not yet sent"); + debug_assert!(false); return }, PendingOutboundPayment::InvoiceReceived { .. } => {