Add pay_for_bolt12_invoice for externally-sourced BOLT 12 invoices#4585
Add pay_for_bolt12_invoice for externally-sourced BOLT 12 invoices#4585Alkamal01 wants to merge 3 commits into
Conversation
…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.
|
I've assigned @valentinewallace as a reviewer! |
| if send_amount > invoice_amount { | ||
| return Err(Bolt12PaymentError::InvalidAmount); | ||
| } |
There was a problem hiding this comment.
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:
| if send_amount > invoice_amount { | |
| return Err(Bolt12PaymentError::InvalidAmount); | |
| } | |
| if send_amount == 0 || send_amount > invoice_amount { | |
| return Err(Bolt12PaymentError::InvalidAmount); | |
| } |
| /// 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); | ||
| } |
There was a problem hiding this comment.
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.
| Err(Bolt12PaymentError::InvalidAmount) => { | ||
| log_error!($logger, "Got InvalidAmount paying internally-sourced invoice; this shouldn't happen"); | ||
| return None | ||
| }, |
There was a problem hiding this comment.
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).
| 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 | |
| }, |
|
The codebase is well-structured. I've now examined every file and hunk in the diff thoroughly. All prior review issues have been addressed in the current revision. No new bugs, security vulnerabilities, or logic errors found. Review SummaryAll five issues from my prior review have been addressed in the current revision:
Verification of core correctness
No new issues found. |
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- 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`
| /// 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, |
There was a problem hiding this comment.
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.
| /// 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, |
| /// Returns [`Bolt12PaymentError::DuplicateInvoice`] if a payment with the given `payment_id` | ||
| /// is already pending, or [`Bolt12PaymentError::InvalidAmount`] if `amount_msats` exceeds the | ||
| /// invoice amount. |
There was a problem hiding this comment.
Same doc inaccuracy as on the InvalidAmount variant: should also mention that zero amount_msats is rejected.
| /// 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.
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
| /// | ||
| /// Either [`Event::PaymentSent`] or [`Event::PaymentFailed`] will be generated once the | ||
| /// payment completes. | ||
| pub fn pay_for_bolt12_invoice( |
There was a problem hiding this comment.
Let's at least deprecate send_payment_for_bolt12_invoice in the same PR.
| /// 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 |
There was a problem hiding this comment.
This should be in the optional params.
4 participants
Add
pay_for_bolt12_invoicefor externally-sourced BOLT 12 invoicesCloses #4380.
send_payment_for_bolt12_invoicecurrently requires a prior LDK-managed offer flow. It verifies the invoice against a pendingpayment_idand fails withUnexpectedInvoiceif none exists.This prevents:
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 apayment_idand handle any invoice validation.An optional
amount_msatsallows 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.