Skip to content

Include routing failures in Bolt12PaymentError #3171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 30, 2024
Merged

Include routing failures in Bolt12PaymentError #3171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa55366
Add Bolt12PaymentError::SendingFailed variant
jkczyz Jul 10, 2024
c8fd681
Refactor initial route finding to separate method
jkczyz Jul 11, 2024
23a1eee
Refactor send_payment_for_bolt12_invoice
jkczyz Jul 11, 2024
b697fbe
Add debug_assert!(false) for unreachable code
jkczyz Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lightning/src/ln/max_payment_path_len_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
158 changes: 104 additions & 54 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -803,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);

Expand All @@ -842,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::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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth implementing From<RetryableSendFailure> for PaymentFailureReason? Or wouldn't it be reusable outside of this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I considered it but it seemed a bit out of place importing RetryableSendFailure in the events module. Might be worth taking a look at how to we might better organize these errors given some of the duplication between RetryableSendFailure and Bolt12PaymentError along with the event reason.

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(())
}

Expand Down Expand Up @@ -928,25 +968,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<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>(
&self, payment_id: PaymentId, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
keysend_preimage: Option<PaymentPreimage>, retry_strategy: Retry, 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,
) -> Result<(), RetryableSendFailure>
fn find_initial_route<R: Deref, NS: Deref, IH, L: Deref>(
&self, payment_id: PaymentId, payment_hash: PaymentHash,
recipient_onion: &RecipientOnionFields, keysend_preimage: Option<PaymentPreimage>,
route_params: &mut RouteParameters, router: &R, first_hops: &Vec<ChannelDetails>,
inflight_htlcs: &IH, node_signer: &NS, best_block_height: u32, logger: &L,
) -> Result<Route, RetryableSendFailure>
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) {
Expand All @@ -957,7 +989,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 \
Expand All @@ -966,7 +998,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::<Vec<_>>()), inflight_htlcs(),
payment_hash, payment_id,
).map_err(|_| {
Expand All @@ -975,12 +1007,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<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>(
&self, payment_id: PaymentId, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
keysend_preimage: Option<PaymentPreimage>, retry_strategy: Retry, 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,
) -> 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)
Expand Down Expand Up @@ -1115,23 +1175,13 @@ impl OutboundPayments {
},
PendingOutboundPayment::AwaitingInvoice { .. } => {
log_error!(logger, "Payment not yet sent");
debug_assert!(false);
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this log include the payment hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could though, I'm wondering also if we should make this a log_trace. Given we'll send multiple invoice requests, we'll likely see multiple invoices even though we'll only handle the first one. The others may hit this case since the lock is released after transition to this state while path finding but before the payment is made.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was mistaken. We should never hit this case just like PendingOutboundPayment::AwaitingInvoice. When we release the lock in send_payment_for_bolt12_invoice, the payment will have transitioned to PendingOutboundPayment::AwaitingInvoice. So attempting to handle another invoice with the same id will cause us to return Bolt12PaymentError::DuplicateInvoice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh, should this be a debug_assert then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and did the same for PendingOutboundPayment::AwaitingInvoice.

debug_assert!(false);
return
},
PendingOutboundPayment::Fulfilled { .. } => {
log_error!(logger, "Payment already completed");
Expand Down Expand Up @@ -2273,7 +2323,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());

Expand Down Expand Up @@ -2334,7 +2384,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());

Expand Down
Loading