Skip to content

Various BOLT11 payment interface fixes #3810

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Various BOLT11 payment interface fixes #3810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7175ba4
Expose `Bolt11PaymentError`
tnull May 21, 2025
864271c
Derive `Debug` for `RouteParametersConfig`
tnull May 21, 2025
9dd44cb
Allow for BOLT11 overpayments again
tnull May 29, 2025
b4da06c
Drop risky `unwrap`s in `PaymentParameters::from_*` methods
tnull May 29, 2025
b655d10
f oops
tnull May 29, 2025
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
13 changes: 9 additions & 4 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use crate::ln::onion_utils::{HTLCFailReason, LocalHTLCFailureReason};
use crate::ln::msgs::{BaseMessageHandler, ChannelMessageHandler, CommitmentUpdate, DecodeError, LightningError, MessageSendEvent};
#[cfg(test)]
use crate::ln::outbound_payment;
use crate::ln::outbound_payment::{Bolt11PaymentError, OutboundPayments, PendingOutboundPayment, RetryableInvoiceRequest, SendAlongPathArgs, StaleExpiration};
use crate::ln::outbound_payment::{OutboundPayments, PendingOutboundPayment, RetryableInvoiceRequest, SendAlongPathArgs, StaleExpiration};
use crate::offers::invoice::{Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSigningPubkey, ExplicitSigningPubkey, InvoiceBuilder, UnsignedBolt12Invoice};
use crate::offers::invoice_error::InvoiceError;
use crate::offers::invoice_request::{InvoiceRequest, InvoiceRequestBuilder};
Expand Down Expand Up @@ -131,7 +131,7 @@ use core::time::Duration;
use core::ops::Deref;
use bitcoin::hex::impl_fmt_traits;
// Re-export this for use in the public API.
pub use crate::ln::outbound_payment::{Bolt12PaymentError, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields};
pub use crate::ln::outbound_payment::{Bolt11PaymentError, Bolt12PaymentError, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields};
#[cfg(any(test, feature = "_externalize_tests"))]
pub(crate) use crate::ln::outbound_payment::PaymentSendFailure;
use crate::ln::script::ShutdownScript;
Expand Down Expand Up @@ -4864,10 +4864,11 @@ where
///
/// # Handling Invoice Amounts
/// Some invoices include a specific amount, while others require you to specify one.
/// - If the invoice **includes** an amount, user must not provide `amount_msats`.
/// - If the invoice **includes** an amount, user may provide an amount greater or equal to it
/// to allow for overpayments.
/// - If the invoice **doesn't include** an amount, you'll need to specify `amount_msats`.
///
/// If these conditions aren’t met, the function will return `Bolt11PaymentError::InvalidAmount`.
/// If these conditions aren’t met, the function will return [`Bolt11PaymentError::InvalidAmount`].
///
/// # Custom Routing Parameters
/// Users can customize routing parameters via [`RouteParametersConfig`].
Expand Down Expand Up @@ -12517,6 +12518,10 @@ where
);
InvoiceError::from(Bolt12SemanticError::UnknownRequiredFeatures)
},
Err(Bolt12PaymentError::InvalidInvoice) => {
log_trace!($logger, "Failed paying invalid/incompatible invoice");
return None;
},
Err(Bolt12PaymentError::SendingFailed(e)) => {
log_trace!($logger, "Failed paying invoice: {:?}", e);
InvoiceError::from_string(format!("{:?}", e))
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/offers_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,7 +1186,7 @@ fn pays_bolt12_invoice_asynchronously() {
let onion_message = alice.onion_messenger.next_onion_message_for_peer(bob_id).unwrap();
bob.onion_messenger.handle_onion_message(alice_id, &onion_message);

// Re-process the same onion message to ensure idempotency —
// Re-process the same onion message to ensure idempotency —
// we should not generate a duplicate `InvoiceReceived` event.
bob.onion_messenger.handle_onion_message(alice_id, &onion_message);

Expand Down Expand Up @@ -2302,7 +2302,7 @@ fn rejects_keysend_to_non_static_invoice_path() {

// Pay the invoice via keysend now that we have the preimage and make sure the recipient fails it
// due to incorrect payment context.
let pay_params = PaymentParameters::from_bolt12_invoice(&invoice);
let pay_params = PaymentParameters::from_bolt12_invoice(&invoice).unwrap();
let route_params = RouteParameters::from_payment_params_and_value(pay_params, amt_msat);
let keysend_payment_id = PaymentId([2; 32]);
let payment_hash = nodes[0].node.send_spontaneous_payment(
Expand Down
22 changes: 17 additions & 5 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,12 +588,15 @@ pub(crate) enum PaymentSendFailure {
#[derive(Debug)]
pub enum Bolt11PaymentError {
/// Incorrect amount was provided to [`ChannelManager::pay_for_bolt11_invoice`].
/// This happens when an amount is specified when [`Bolt11Invoice`] already contains
/// an amount, or vice versa.
/// This happens when the user-provided amount is less than an amount specified in the [`Bolt11Invoice`].
///
/// [`Bolt11Invoice`]: lightning_invoice::Bolt11Invoice
/// [`ChannelManager::pay_for_bolt11_invoice`]: crate::ln::channelmanager::ChannelManager::pay_for_bolt11_invoice
InvalidAmount,
/// An invalid or incompatible invoice was provided to [`ChannelManager::pay_for_bolt11_invoice`].
///
/// [`ChannelManager::pay_for_bolt11_invoice`]: crate::ln::channelmanager::ChannelManager::pay_for_bolt11_invoice
InvalidInvoice,
Comment on lines +596 to +599
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to add this if it will never be created? It should be an invariant of from_bolt11_invoice that it will never fail. We can change the unwraps to expects, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I agree these are not reachable at the moment, but it's pretty risky to have unwraps and expects sprinkled over the codebase that could get reachable, e.g., by a refactoring in the future (it would just take introducing one more error condition that depends on the user input in with_route_hints or with_{bolt11,bolt12}_features and we'd get reachable panics in the corresponding from_ method(s)).
IMO, unwraps (outside of trivial ones like on lock), should generally be avoided.

If you dislike the extra error variant I guess we could also see if there are other ways to make this safer (more comments, debug_asserts, maybe tests), or through the type system, so open for suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, ideally through the type system where the Payee enum is pulled out of PaymentParameters. Probably something like:

pub enum Payee {
	Bolt11(Bolt11Payee),
	Bolt12(Bolt12Payee),
}

pub struct Bolt11Payee {
	node_id: PublicKey,
	route_hints: Vec<RouteHint>,
	features: Option<Bolt11InvoiceFeatures>,
	final_cltv_expiry_delta: u32,
	params: PaymentParameters,
}

pub struct Bolt12Payee {
	route_hints: Vec<BlindedPaymentPath>,
	features: Option<Bolt12InvoiceFeatures>,
	params: PaymentParameters,
}

And where RouteParameters holds a Payee instead of PaymentParameters.

/// The invoice was valid for the corresponding [`PaymentId`], but sending the payment failed.
SendingFailed(RetryableSendFailure),
}
Expand All @@ -605,6 +608,10 @@ pub enum Bolt12PaymentError {
UnexpectedInvoice,
/// Payment for an invoice with the corresponding [`PaymentId`] was already initiated.
DuplicateInvoice,
/// An invalid or incompatible invoice was provided to [`ChannelManager`].
///
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
InvalidInvoice,
Comment on lines +611 to +614
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise.

/// The invoice was valid for the corresponding [`PaymentId`], but required unknown features.
UnknownRequiredFeatures,
/// The invoice was valid for the corresponding [`PaymentId`], but sending the payment failed.
Expand Down Expand Up @@ -894,13 +901,16 @@ impl OutboundPayments {

let amount = match (invoice.amount_milli_satoshis(), amount_msats) {
(Some(amt), None) | (None, Some(amt)) => amt,
(None, None) | (Some(_), Some(_)) => return Err(Bolt11PaymentError::InvalidAmount),
(Some(inv_amt), Some(user_amt)) if user_amt < inv_amt => return Err(Bolt11PaymentError::InvalidAmount),
(Some(_), Some(user_amt)) => user_amt,
(None, None) => return Err(Bolt11PaymentError::InvalidAmount),
};

let mut recipient_onion = RecipientOnionFields::secret_only(*invoice.payment_secret());
recipient_onion.payment_metadata = invoice.payment_metadata().map(|v| v.clone());

let payment_params = PaymentParameters::from_bolt11_invoice(invoice)
.map_err(|_| Bolt11PaymentError::InvalidInvoice)?
.with_user_config_ignoring_fee_limit(route_params_config);

let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amount);
Expand Down Expand Up @@ -948,6 +958,7 @@ impl OutboundPayments {

let mut route_params = RouteParameters::from_payment_params_and_value(
PaymentParameters::from_bolt12_invoice(&invoice)
.map_err(|_| Bolt12PaymentError::InvalidInvoice)?
.with_user_config_ignoring_fee_limit(params_config), invoice.amount_msats()
);
if let Some(max_fee_msat) = params_config.max_total_routing_fee_msat {
Expand Down Expand Up @@ -1126,6 +1137,7 @@ impl OutboundPayments {
let keysend_preimage = PaymentPreimage(entropy_source.get_secure_random_bytes());
let payment_hash = PaymentHash(Sha256::hash(&keysend_preimage.0).to_byte_array());
let pay_params = PaymentParameters::from_static_invoice(invoice)
.map_err(|_| Bolt12PaymentError::InvalidInvoice)?
.with_user_config_ignoring_fee_limit(*route_params_config);
let mut route_params = RouteParameters::from_payment_params_and_value(pay_params, amount_msat);
route_params.max_total_routing_fee_msat = route_params_config.max_total_routing_fee_msat;
Expand Down Expand Up @@ -3033,7 +3045,7 @@ mod tests {
assert!(outbound_payments.has_pending_payments());

let route_params = RouteParameters::from_payment_params_and_value(
PaymentParameters::from_bolt12_invoice(&invoice),
PaymentParameters::from_bolt12_invoice(&invoice).unwrap(),
invoice.amount_msats(),
);
router.expect_find_route(route_params, Err(""));
Expand Down Expand Up @@ -3085,7 +3097,7 @@ mod tests {
.sign(recipient_sign).unwrap();

let route_params = RouteParameters {
payment_params: PaymentParameters::from_bolt12_invoice(&invoice),
payment_params: PaymentParameters::from_bolt12_invoice(&invoice).unwrap(),
final_value_msat: invoice.amount_msats(),
max_total_routing_fee_msat: Some(1234),
};
Expand Down
33 changes: 19 additions & 14 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,41 +921,46 @@ impl PaymentParameters {
/// Creates parameters for paying to a blinded payee from the provided invoice. Sets
/// [`Payee::Blinded::route_hints`], [`Payee::Blinded::features`], and
/// [`PaymentParameters::expiry_time`].
pub fn from_bolt11_invoice(invoice: &Bolt11Invoice) -> Self {
///
/// Will return an error if an incompatible invoice is provided.
pub fn from_bolt11_invoice(invoice: &Bolt11Invoice) -> Result<Self, ()>{
let mut payment_params = Self::from_node_id(
invoice.recover_payee_pub_key(),
invoice.min_final_cltv_expiry_delta() as u32,
)
.with_route_hints(invoice.route_hints())
.unwrap();
.with_route_hints(invoice.route_hints())?;

if let Some(expiry) = invoice.expires_at() {
payment_params = payment_params.with_expiry_time(expiry.as_secs());
}
if let Some(features) = invoice.features() {
payment_params = payment_params.with_bolt11_features(features.clone()).unwrap();
payment_params = payment_params.with_bolt11_features(features.clone())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, but neither of these calls an fail, right? Why bother returning a result for an infallible operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently. But it just takes one added validation step on the user input in another part of the code for this to transform into a reachable panic. IMO we should get rid of any unwraps / expects, especially ones that could easily get hit as a consequence of a simple refactoring in another place.

}

payment_params
Ok(payment_params)
}

/// Creates parameters for paying to a blinded payee from the provided invoice. Sets
/// [`Payee::Blinded::route_hints`], [`Payee::Blinded::features`], and
/// [`PaymentParameters::expiry_time`].
pub fn from_bolt12_invoice(invoice: &Bolt12Invoice) -> Self {
Self::blinded(invoice.payment_paths().to_vec())
.with_bolt12_features(invoice.invoice_features().clone()).unwrap()
.with_expiry_time(invoice.created_at().as_secs().saturating_add(invoice.relative_expiry().as_secs()))
///
/// Will return an error if an incompatible invoice is provided.
pub fn from_bolt12_invoice(invoice: &Bolt12Invoice) -> Result<Self, ()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly, why is this public to begin with? Presumably no one should ever use it directly?

Copy link
Contributor Author

@tnull tnull Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In the manually_handle_bolt12 flow we need to call send_payment_for_bolt12_invoice. While this currently doesn't allow to customize anything, we arguably should allow for it, i.e., at the very least allow to set RouteParametersConfig as we do for the other send APIs now. If we we think we'd rather need the 'legacy' way of RouteParameters/PaymentParameters, we'd probably need this API.

I think we'd currently also need it to send probes for a Bolt12Invoice.

Ok(Self::blinded(invoice.payment_paths().to_vec())
.with_bolt12_features(invoice.invoice_features().clone())?
.with_expiry_time(invoice.created_at().as_secs().saturating_add(invoice.relative_expiry().as_secs())))
}

/// Creates parameters for paying to a blinded payee from the provided invoice. Sets
/// [`Payee::Blinded::route_hints`], [`Payee::Blinded::features`], and
/// [`PaymentParameters::expiry_time`].
///
/// Will return an error if an incompatible invoice is provided.
#[cfg(async_payments)]
pub fn from_static_invoice(invoice: &StaticInvoice) -> Self {
Self::blinded(invoice.payment_paths().to_vec())
.with_bolt12_features(invoice.invoice_features().clone()).unwrap()
.with_expiry_time(invoice.created_at().as_secs().saturating_add(invoice.relative_expiry().as_secs()))
pub fn from_static_invoice(invoice: &StaticInvoice) -> Result<Self, ()> {
Ok(Self::blinded(invoice.payment_paths().to_vec())
.with_bolt12_features(invoice.invoice_features().clone())?
.with_expiry_time(invoice.created_at().as_secs().saturating_add(invoice.relative_expiry().as_secs())))
}

/// Creates parameters for paying to a blinded payee from the provided blinded route hints.
Expand Down Expand Up @@ -1075,7 +1080,7 @@ impl PaymentParameters {
}

/// A struct for configuring parameters for routing the payment.
#[derive(Clone, Copy)]
#[derive(Clone, Copy, Debug)]
pub struct RouteParametersConfig {
/// The maximum total fees, in millisatoshi, that may accrue during route finding.
///
Expand Down
Loading