Skip to content

Add pay_for_bolt12_invoice for externally-sourced BOLT 12 invoices#4585

Open
Alkamal01 wants to merge 4 commits into
lightningdevkit:mainfrom
Alkamal01:bolt12-partial-mpp-payment
Open

Add pay_for_bolt12_invoice for externally-sourced BOLT 12 invoices#4585
Alkamal01 wants to merge 4 commits into
lightningdevkit:mainfrom
Alkamal01:bolt12-partial-mpp-payment

Conversation

@Alkamal01

Copy link
Copy Markdown
Contributor

Add pay_for_bolt12_invoice for externally-sourced BOLT 12 invoices

Closes #4380.

send_payment_for_bolt12_invoice currently requires a prior LDK-managed offer flow. It verifies the invoice against a pending payment_id and fails with UnexpectedInvoice if none exists.

This prevents:

  • paying BOLT 12 invoices obtained out-of-band
  • splitting a single invoice payment across multiple senders

This PR introduces ChannelManager::pay_for_bolt12_invoice, which allows paying such invoices without a prior offer flow. It skips LDK-side verification, so the caller must provide a payment_id and handle any invoice validation.

An optional amount_msats allows partial payments. This enables multi-sender flows, where each node contributes a portion of the total. The routed amount reflects the contribution, while the onion total remains the full invoice amount so the recipient can validate it correctly.

…nvoices

Adds ChannelManager::pay_for_bolt12_invoice which pays a Bolt12Invoice
without requiring a prior LDK-managed request. The caller provides their
own payment_id and is responsible for verification, enabling partial MPP
payments from multiple senders.

The onion total is always set to the full invoice amount so recipients
can validate correctly regardless of each sender's contribution.
@ldk-reviews-bot

ldk-reviews-bot commented May 2, 2026

Copy link
Copy Markdown

I've assigned @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Comment thread lightning/src/ln/outbound_payment.rs Outdated
Comment on lines +1160 to +1162
if send_amount > invoice_amount {
return Err(Bolt12PaymentError::InvalidAmount);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Bug: amount_msats = Some(0) passes this check (0 <= invoice_amount), which inserts an InvoiceReceived entry and then proceeds to route finding for 0 msat. Routing will fail, triggering abandon_payment and a PaymentFailed event, but a zero-value payment is never meaningful and should be rejected upfront.

Consider:

Suggested change
if send_amount > invoice_amount {
return Err(Bolt12PaymentError::InvalidAmount);
}
if send_amount == 0 || send_amount > invoice_amount {
return Err(Bolt12PaymentError::InvalidAmount);
}

Comment on lines +2671 to +2733
/// Checks that a BOLT 12 invoice can be paid via [`ChannelManager::pay_for_bolt12_invoice`]
/// without requiring a prior LDK-managed payment request.
#[test]
fn pay_for_bolt12_invoice_with_fresh_payment_id() {
let mut manually_pay_cfg = test_default_channel_config();
manually_pay_cfg.manually_handle_bolt12_invoices = true;

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, Some(manually_pay_cfg)]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000);

let alice = &nodes[0];
let alice_id = alice.node.get_our_node_id();
let bob = &nodes[1];
let bob_id = bob.node.get_our_node_id();

let offer = alice.node
.create_offer_builder().unwrap()
.amount_msats(10_000_000)
.build().unwrap();

// Use the standard offer flow to obtain an invoice, but pay it via the new API with a
// fresh payment_id rather than the one from the original request.
let orig_payment_id = PaymentId([1; 32]);
bob.node.pay_for_offer(&offer, None, orig_payment_id, Default::default()).unwrap();

let onion_message = bob.onion_messenger.next_onion_message_for_peer(alice_id).unwrap();
alice.onion_messenger.handle_onion_message(bob_id, &onion_message);

let (invoice_request, _) = extract_invoice_request(alice, &onion_message);
let payment_context = PaymentContext::Bolt12Offer(Bolt12OfferContext {
offer_id: offer.id(),
invoice_request: InvoiceRequestFields {
payer_signing_pubkey: invoice_request.payer_signing_pubkey(),
quantity: None,
payer_note_truncated: None,
human_readable_name: None,
},
});

let onion_message = alice.onion_messenger.next_onion_message_for_peer(bob_id).unwrap();
bob.onion_messenger.handle_onion_message(alice_id, &onion_message);

let invoice = match get_event!(bob, Event::InvoiceReceived) {
Event::InvoiceReceived { invoice, .. } => invoice,
_ => panic!("Expected InvoiceReceived"),
};

// Abandon the original payment since we're paying via a fresh payment_id below.
bob.node.abandon_payment(orig_payment_id);
get_event!(bob, Event::PaymentFailed);

let payment_id = PaymentId([2; 32]);
bob.node.pay_for_bolt12_invoice(&invoice, payment_id, None, Default::default()).unwrap();
expect_recent_payment!(bob, RecentPaymentDetails::Pending, payment_id);

route_bolt12_payment(bob, &[alice], &invoice);
claim_bolt12_payment(bob, &[alice], payment_context, &invoice);
expect_recent_payment!(bob, RecentPaymentDetails::Fulfilled, payment_id);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Missing test coverage: Neither this test nor pay_for_bolt12_invoice_error_cases actually exercises the partial payment path (amount_msats = Some(partial_amount)) end-to-end. This is the primary feature of this PR — enabling multi-sender split payments where each node pays a portion of the invoice.

A test should verify that paying with e.g. Some(invoice.amount_msats()) (or a true partial amount in a multi-node setup) correctly sets total_mpp_amount_msat in the onion to the full invoice amount while routing only the partial amount. Without this, the total_mpp_amount_msat_override plumbing through send_payment_for_bolt12_invoice_internal is untested.

Comment on lines +17133 to +17136
Err(Bolt12PaymentError::InvalidAmount) => {
log_error!($logger, "Got InvalidAmount paying internally-sourced invoice; this shouldn't happen");
return None
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: this arm is currently unreachable since internally-sourced invoices go through send_payment_for_bolt12_invoice, which never returns InvalidAmount. Adding a debug_assert!(false) would make this more robust against future changes that might inadvertently make this path reachable without proper cleanup (the payment would be stuck in InvoiceReceived state since no PaymentFailed event is generated here).

Suggested change
Err(Bolt12PaymentError::InvalidAmount) => {
log_error!($logger, "Got InvalidAmount paying internally-sourced invoice; this shouldn't happen");
return None
},
Err(Bolt12PaymentError::InvalidAmount) => {
log_error!($logger, "Got InvalidAmount paying internally-sourced invoice; this shouldn't happen");
debug_assert!(false);
return None
},

@ldk-claude-review-bot

ldk-claude-review-bot commented May 2, 2026

Copy link
Copy Markdown
Collaborator

The commit (095030bf1) is identical to my prior review pass, and the diff is unchanged. I re-examined the full diff, the partial-payment plumbing, the route-parameter construction, error handling/cleanup paths, and the MPP routing logic. I did not find any new issues beyond those already flagged in my previous review.

Review Summary

No new issues found. The commit is unchanged since my prior review pass, so all previously raised findings still stand (and remain unaddressed):

  • lightning/src/ln/channelmanager.rs:5862 — Misleading deprecation note: pay_for_bolt12_invoice is not a drop-in replacement for send_payment_for_bolt12_invoice (requires a fresh payment_id; reusing the Event::InvoiceReceived id yields DuplicateInvoice).
  • lightning/src/ln/outbound_payment.rs:1162 / :665, channelmanager.rs:5900, :17146 — Prior nits on the zero-amount rejection, InvalidAmount doc wording, and the debug_assert! placement.
  • Cross-cutting: events/mod.rs:1021 and util/config.rs:1084 docs still steer users to the now-deprecated method (lines not in diff).

Newly verified during this pass (no issue raised):

  • The partial-payment path (amount_msats = Some(partial)) is now covered by the pay_for_bolt12_invoice_partial_amount test, addressing my prior "missing test coverage" comment.
  • Partial MPP sends rely on the recipient supporting basic_mpp; the router's allow_mpp check only governs sender-side path splitting, not the cross-sender total. This is the caller's coordination responsibility per the documented multi-sender use case, so I did not flag it as a bug.

@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.91837% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.16%. Comparing base (6573d42) to head (dbc0169).
⚠️ Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/outbound_payment.rs 94.33% 1 Missing and 2 partials ⚠️
lightning/src/ln/channelmanager.rs 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4585      +/-   ##
==========================================
+ Coverage   86.99%   87.16%   +0.17%     
==========================================
  Files         163      161       -2     
  Lines      109008   109343     +335     
  Branches   109008   109343     +335     
==========================================
+ Hits        94828    95314     +486     
+ Misses      11696    11551     -145     
+ Partials     2484     2478       -6     
Flag Coverage Δ
fuzzing ?
fuzzing-fake-hashes 31.17% <10.20%> (?)
fuzzing-real-hashes 22.90% <10.20%> (?)
tests 86.23% <95.91%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Reject `send_amount == 0` in `pay_for_bolt12_invoice`
- Add `debug_assert!(false)` in `InvalidAmount` arm of `handle_pay_invoice_res!`
  (unreachable for internally-sourced invoices)
- Add `pay_for_bolt12_invoice_partial_amount` test verifying:
  - HTLC `amount_msat` equals `partial_amount`
  - `total_mpp_amount_msat` in the onion equals full invoice amount
  - recipient holds HTLC rather than settling
- Add zero-amount assertion to `pay_for_bolt12_invoice_error_cases`
Comment on lines +660 to +665
/// Incorrect amount was provided to [`ChannelManager::pay_for_bolt12_invoice`].
///
/// This occurs when `amount_msats` exceeds the invoice amount.
///
/// [`ChannelManager::pay_for_bolt12_invoice`]: crate::ln::channelmanager::ChannelManager::pay_for_bolt12_invoice
InvalidAmount,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: The doc says "This occurs when amount_msats exceeds the invoice amount" but the actual validation (send_amount == 0 || send_amount > invoice_amount) also rejects zero amounts. The same incomplete description appears in the pay_for_bolt12_invoice method doc at channelmanager.rs:5899.

Suggested change
/// Incorrect amount was provided to [`ChannelManager::pay_for_bolt12_invoice`].
///
/// This occurs when `amount_msats` exceeds the invoice amount.
///
/// [`ChannelManager::pay_for_bolt12_invoice`]: crate::ln::channelmanager::ChannelManager::pay_for_bolt12_invoice
InvalidAmount,
/// Incorrect amount was provided to [`ChannelManager::pay_for_bolt12_invoice`].
///
/// This occurs when `amount_msats` is zero or exceeds the invoice amount.
///
/// [`ChannelManager::pay_for_bolt12_invoice`]: crate::ln::channelmanager::ChannelManager::pay_for_bolt12_invoice
InvalidAmount,

Comment thread lightning/src/ln/channelmanager.rs Outdated
Comment on lines +5898 to +5900
/// Returns [`Bolt12PaymentError::DuplicateInvoice`] if a payment with the given `payment_id`
/// is already pending, or [`Bolt12PaymentError::InvalidAmount`] if `amount_msats` exceeds the
/// invoice amount.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same doc inaccuracy as on the InvalidAmount variant: should also mention that zero amount_msats is rejected.

Suggested change
/// Returns [`Bolt12PaymentError::DuplicateInvoice`] if a payment with the given `payment_id`
/// is already pending, or [`Bolt12PaymentError::InvalidAmount`] if `amount_msats` exceeds the
/// invoice amount.
/// Returns [`Bolt12PaymentError::DuplicateInvoice`] if a payment with the given `payment_id`
/// is already pending, or [`Bolt12PaymentError::InvalidAmount`] if `amount_msats` is zero or
/// exceeds the invoice amount.

Update InvalidAmount docs to explicitly mention zero amount rejection, and add missing test coverage for the UnknownRequiredFeatures error during BOLT 12 invoice payments.
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@TheBlueMatt TheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

///
/// Either [`Event::PaymentSent`] or [`Event::PaymentFailed`] will be generated once the
/// payment completes.
pub fn pay_for_bolt12_invoice(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's at least deprecate send_payment_for_bolt12_invoice in the same PR.

Comment thread lightning/src/ln/channelmanager.rs Outdated
/// that the invoice was previously requested. The caller is responsible for invoice
/// verification and for providing a unique `payment_id`.
///
/// `amount_msats` controls how much this node contributes to the payment. Set to `None` to pay

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be in the optional params.

Comment thread lightning/src/ln/channelmanager.rs Outdated
/// whether or not the payment was successful.
///
/// [timer tick]: Self::timer_tick_occurred
#[deprecated(since = "0.3.0", note = "Use ChannelManager::pay_for_bolt12_invoice instead.")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The deprecation note "Use ChannelManager::pay_for_bolt12_invoice instead." is misleading — the two methods are not drop-in replacements:

  1. send_payment_for_bolt12_invoice derives the payment_id from the invoice's payer_metadata (via verify_bolt12_invoice) and pays against the existing pending payment created by the offer flow. pay_for_bolt12_invoice instead requires the caller to supply a fresh, unused payment_id and inserts a new InvoiceReceived entry.

  2. For the primary documented use case (manually_handle_bolt12_invoicesEvent::InvoiceReceived), the payment is already in InvoiceReceived state because mark_invoice_received was called at message-handling time (channelmanager.rs:17268). Calling pay_for_bolt12_invoice with that same payment_id will hit the Occupied branch and return Bolt12PaymentError::DuplicateInvoice.

So a user who follows this deprecation guidance for the standard InvoiceReceived flow (passing the event's payment_id) will get a DuplicateInvoice error. The test pay_for_bolt12_invoice_with_fresh_payment_id even has to abandon_payment(orig_payment_id) and use a different id, confirming the old id can't be reused.

Consider either keeping send_payment_for_bolt12_invoice non-deprecated (the two serve different flows), or clarifying in the note that the replacement requires a brand-new payment_id and that the invoice must be independently verified by the caller.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Kept the deprecation per @TheBlueMatt ask, but you're right the note read as drop-in. Updated it to call out that pay_for_bolt12_invoice needs a fresh payment_id and caller-side invoice verification.

Move `amount_msats` from a positional parameter of `pay_for_bolt12_invoice`
into `OptionalBolt12PaymentParams` as requested, and deprecate
`send_payment_for_bolt12_invoice` in favor of `pay_for_bolt12_invoice`.
@Alkamal01 Alkamal01 force-pushed the bolt12-partial-mpp-payment branch from 5a62351 to 095030b Compare June 9, 2026 09:17
@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support paying < total MPP value in BOLT 12 flow

4 participants