Support payments for less than the total MPP value#4373
Support payments for less than the total MPP value#4373TheBlueMatt wants to merge 8 commits intolightningdevkit:mainfrom
Conversation
|
👋 I see @tankyleo was un-assigned. |
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
2 times, most recently
from
a4b00d4 to
e8d40e1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4373 +/- ##
==========================================
+ Coverage 86.09% 86.10% +0.01%
==========================================
Files 156 156
Lines 103623 103721 +98
Branches 103623 103721 +98
==========================================
+ Hits 89211 89309 +98
+ Misses 11895 11887 -8
- Partials 2517 2525 +8
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:
|
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
3 times, most recently
from
93228cf to
b6b973f
Compare
|
Opportunistically tagging this 0.3 since its rather important for some of our users (and its important for my wallet project!) |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
| if let Some(proc_macro2::TokenTree::Group(group)) = ty_tokens { | ||
| let first_token = group.stream().into_iter().next(); | ||
| if let Some(proc_macro2::TokenTree::Ident(ident)) = first_token { | ||
| if is_init && ident == "legacy" { |
There was a problem hiding this comment.
Is this also suppose to check for "custom"? Without doing so, I'm unable to use this for a legacy field where it needs to be read in order to update another field.
error[E0560]: struct `FundingTxInput` has no field named `_sequence`
--> lightning/src/ln/funding.rs:120:6
|
120 | (3, _sequence, (custom, Sequence,
| ^^^^^^^^^ `FundingTxInput` does not have this field
|
= note: all struct fields are already assigned
For more information about this error, try `rustc --explain E0560`.
error: could not compile `lightning` (lib) due to previous error
This is what I tried in #4290.
diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs
index 3b7c0bedb..2de763118 100644
--- a/lightning/src/ln/funding.rs
+++ b/lightning/src/ln/funding.rs
@@ -117,7 +117,19 @@ pub struct FundingTxInput {
impl_writeable_tlv_based!(FundingTxInput, {
(1, utxo, required),
- (3, _sequence, (legacy, Sequence, |input: &FundingTxInput| Some(input.utxo.sequence))),
+ (3, _sequence, (custom, Sequence,
+ |read_val: Option<Sequence>| {
+ if let Some(sequence) = read_val {
+ // Utxo contains sequence now, so update it if the value read here differs since
+ // this indicates Utxo::sequence was read with the default_value
+ let utxo: &mut Utxo = utxo.0.as_mut().expect("utxo is required");
+ if utxo.sequence != sequence {
+ utxo.sequence = sequence;
+ }
+ }
+ Ok(read_val)
+ },
+ |input: &FundingTxInput| Some(input.utxo.sequence))),
(5, prevtx, required),
});
There was a problem hiding this comment.
Hmm, no, grrrrrrr. I was using custom for the "we have a field we need to initialize, but we need to do so via some special logic" rather than what you really needed which is "we have removed this field and want to have some logic when reading its old TLV". I think what we actually need is a read method to legacy that works like the custom case but is ignored for field-initialization cases.
There was a problem hiding this comment.
What about checking if the field started with an underscore? Too risky / non-obvious?
There was a problem hiding this comment.
Yea, seems weird. legacy was kinda all about "item removed" whereas custom is about "item needs special handling". IMO it makes sense to have a method called on legacy after its read.
wpaulino
requested review from
valentinewallace
and removed request for
wpaulino
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Needs rebase, getting comments out so GH doesn't eat them
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
2 times, most recently
from
2abd54b to
3d72f76
Compare
|
Rebased and addressed feedback. Note that I noticed the CLTV delta stuff for trampoline is a bit wrong so that's not checked here. |
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
2 times, most recently
from
1199749 to
db6aa28
Compare
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
from
db6aa28 to
d0a914c
Compare
|
Rebased without conflicts, not sure why github thought this had conflicts. Because #4402 is now built on this would be good to move quicker. |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Basically looks good! I haven't looked at the mechanical changes too closely. Fuzz CI needs a fix.
lightning/src/ln/outbound_payment.rs
Outdated
| if mpp_amt < amount { | ||
| return Err(Bolt11PaymentError::InvalidAmount); | ||
| } | ||
| if let Some(invoice_amount) = invoice.amount_milli_satoshis() { | ||
| if mpp_amt < invoice_amount { | ||
| return Err(Bolt11PaymentError::InvalidAmount); | ||
| } |
There was a problem hiding this comment.
We don't have test coverage of the error cases, just the happy path. Also missing coverage of the retries portion. Could also be interesting to test MPP-within-the-multi-node-MPP?
There was a problem hiding this comment.
Sure, I had claude write a test that just does a simple retry. I didn't bother trying to push it to do an MPP but could ask if it you feel strongly on it - i figured it didn't matter.
| payment_metadata: payment_metadata.clone(), | ||
| custom_tlvs: custom_tlvs.clone(), | ||
| total_mpp_amount_msat: *total_msat, | ||
| total_mpp_amount_msat: *onion_total_msat, |
There was a problem hiding this comment.
It's somewhat out-of-scope, but we could prevent subtle bugs like this by encoding the entire RecipientOnionFields in the ::Retryable variant instead of the individual fields.
There was a problem hiding this comment.
Yea, I also thought about that. Its already "wrong", though, and it didn't feel urgent to address.
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
from
85686ab to
e5b4e4b
Compare
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
2 times, most recently
from
aaf4450 to
5427994
Compare
|
Squashed fixups but had to rebase again as a new test was added that needed updating... |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
LGTM. Definitely needs a second reviewer
| } else { | ||
| // LDK versions prior to 0.0.107 did not write a `pending_htlc_purposes`, but do | ||
| // include a `_legacy_hop_data` in the `OnionPayload`. | ||
| for (payment_hash, htlcs) in claimable_htlcs_list.into_iter() { |
There was a problem hiding this comment.
Nice to drop all this code
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
from
5427994 to
0819091
Compare
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tankyleo! This PR has been waiting for your review. |
| route: &mut Route, route_params: &RouteParameters, logger: &WithContext<L>, | ||
| ) -> Result<(), ()> { | ||
| if route.route_params.as_ref() != Some(route_params) { | ||
| debug_assert!( |
There was a problem hiding this comment.
pre-existing, but why don't we just error out here to fail the payment?
There was a problem hiding this comment.
It indicates a bug in the Router implementation that was provided to us. In debug builds we prefer to surface an implementation issue as a panic.
lightning/src/ln/onion_utils.rs
Outdated
| }; | ||
| let mut outer_onion = recipient_onion; |
There was a problem hiding this comment.
Branch rather than making outer_onion mut and overriding it?
let has_trampoline_hops = path.blinded_tail.as_ref().is_some_and(|tail| !tail.trampoline_hops.is_empty());
let (outer_onion, trampoline_packet_option, outer_starting_htlc_offset) = if has_trampoline_hops
{
{build trampoline packet as currently}
} else {
(recipient_onion, None, cur_block_height)
};
There was a problem hiding this comment.
Didn't bother with outer_starting_htlc_offset since #4402 drops it.
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
from
0819091 to
9ceb9d6
Compare
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
I did not fully grasp some changes in this PR (esp some parts referencing trampoline) but still wanted to take a look to get an idea of how this feature could be implemented.
This is unrelated but why are fields in RouteParameters struct annotated with "failed payment path"?
send_payment which makes it confusing.
| receiving_channel_ids: claimable_payment.receiving_channel_ids(), | ||
| claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER), | ||
| onion_fields: claimable_payment.onion_fields.clone(), | ||
| onion_fields: Some(claimable_payment.onion_fields.clone()), |
There was a problem hiding this comment.
This could use onion_fields and avoid the clone. IIUC, after check_merge they should be the same since non-matching fields are dropped from both.
There was a problem hiding this comment.
check_merge copies things from onion_fields into claimable_payment.onion_fields, but not the other way around, so that might miss some custom TLVs.
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
fixups lgtm, ready for squash + rebase when others are!
When `OutboundPayments` calls the provided `Router` to fetch a `Route` it passes a `RouteParameters` with a specific max-fee. Here we validate that the `Route` returned sticks to the limits provided, and also that it meets the MPP rules of not having any single MPP part which can be removed while still meeting the desired payment amount.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In order to allow for this, we need to separate the concept of the payment amount from the onion MPP amount. Here we start this process by adding a `total_mpp_amount_msat` field to `RecipientOnionFields` (which is the appropriate place for a field describing something in the recipient onion). We currently always assert that it is equal to the existing fields, but will relax this in the coming commit(s). We also start including a payment preimage on probe attempts, which appears to have been the intent of the code, but which did not work correctly. The bulk of the test updates were done by Claude.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In the previous commit we added a new field to `RecipientOnionFields` to describe the total value of an MPP payment. Here we start using this field when building onions, dropping existing arguments to onion-building methods.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In the previous commits we moved the total-MPP-value we set in onions from being manually passed through onion-building to passing it via `RecipientOnionFields`. This introduced a subtle bug, though - payments which are retried will get a fresh `RecipientOnionFields` built from the data in `PendingOutboundPayment::Retryable`, losing any custom total-MPP-value settings and causing retries to fail. Here we fix this by storing the total-MPP-value directly in `PendingOutboundPayment::Retryable`.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In the previous few commits we added support for making these kinds of payments when using the payment methods which explicitly accepted a `RecipientOnionFields`. Here we also add support for such payments made via the `pay_for_bolt11_invoice` method, utilizing the new `OptionalBolt11PaymentParams` to hide the parameter from most calls. Test mostly by Claude
We added `RecipientOnionFields` in the `ClaimablePayment`/`ClaimingPayment` structs in 0.0.115/0.0.124, always writing them for new HTLCs. As of 0.1, we do not support upgrading from 0.0.123 or earlier with pending HTLCs to forward or claim. Thus, we already don't support upgrading in cases where no `RecipientOnionFields` is set and we can thus go ahead and mark it as non-`Option`al. Further, there's some super ancient upgrade logic in `ChannelManager` deserialization we can remove at the same time.
Now that we have `total_mpp_amount_msat` in the now-required `RecipientOnionFields` in `ClaimablePayment`s, the `total_msat` field in `ClaimableHTLC` is redundant. Given it was already awkward that we stored it in *each` `ClaimableHTLC` despite it being required to match in all of them, its good to drop it.
|
Squashed + rebased with the following changes requested by @elnosh: $ git diff
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 8f86237594..cec6d5b4fd 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -17471,10 +17471,10 @@ impl Readable for VecDeque<(Event, Option<EventCompletionAction>)> {
}
}
-/// We write the [`ClaimableHTLC`] [`RecipientOnionFields`] separately as they were added sometime
-/// later. Because [`ClaimableHTLC`] only implements [`ReadableArgs`] we have to add a wrapper
-/// which reads them without [`RecipientOnionFields::total_mpp_amount_msat`] and then fill them in
-/// later.
+/// We write the [`ClaimableHTLC`]'s [`RecipientOnionFields`] separately as they were added sometime
+/// later. Because [`RecipientOnionFields`] only implements [`ReadableArgs`] we have to add a
+/// wrapper which reads them without [`RecipientOnionFields::total_mpp_amount_msat`] and then fill
+/// them in later.
struct AmountlessClaimablePaymentHTLCOnion(RecipientOnionFields);
impl Readable for AmountlessClaimablePaymentHTLCOnion {
@@ -17796,7 +17796,7 @@ impl<'a, ES: EntropySource, SP: SignerProvider, L: Logger>
}
}
} else if !purposes.is_empty() || !claimable_htlcs_list.is_empty() {
- // `amountless_claimable_htlc_onion_fields` was first written in LDK 0.0.115. We no
+ // `amountless_claimable_htlc_onion_fields` was first written in LDK 0.0.115. We
// haven't supported upgrade from 0.0.115 with pending HTLCs since 0.1.
return Err(DecodeError::InvalidValue);
}
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 797e693655..be167b3bcd 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -5418,8 +5418,7 @@ fn max_out_mpp_path() {
nodes[0].node.get_and_clear_pending_msg_events();
}
-#[test]
-fn bolt11_multi_node_mpp() {
+fn do_bolt11_multi_node_mpp(use_bolt11_pay: bool) {
// Test that multiple nodes can collaborate to pay a single BOLT 11 invoice, with each node
// paying a portion of the total invoice amount. This is useful for scenarios like:
// - Paying from multiple wallets (e.g., ecash wallets with funds in multiple mints)
@@ -5441,18 +5440,28 @@ fn bolt11_multi_node_mpp() {
..Default::default()
};
let invoice = nodes[2].node.create_bolt11_invoice(invoice_params).unwrap();
+ let pmt_hash = invoice.payment_hash();
// Node A pays 60_000 msat (part of the total)
let node_a_payment_amt = 60_000;
let payment_id_a = PaymentId([1; 32]);
- let optional_params_a = crate::ln::channelmanager::OptionalBolt11PaymentParams {
- declared_total_mpp_value_msat_override: Some(invoice_amt_msat),
- ..Default::default()
- };
- nodes[0]
- .node
- .pay_for_bolt11_invoice(&invoice, payment_id_a, Some(node_a_payment_amt), optional_params_a)
- .unwrap();
+ if use_bolt11_pay {
+ let params = crate::ln::channelmanager::OptionalBolt11PaymentParams {
+ declared_total_mpp_value_msat_override: Some(invoice_amt_msat),
+ ..Default::default()
+ };
+ nodes[0]
+ .node
+ .pay_for_bolt11_invoice(&invoice, payment_id_a, Some(node_a_payment_amt), params)
+ .unwrap();
+ } else {
+ let onion = RecipientOnionFields::secret_only(*invoice.payment_secret(), invoice_amt_msat);
+ let pay_params = PaymentParameters::from_bolt11_invoice(&invoice);
+ let route_params =
+ RouteParameters::from_payment_params_and_value(pay_params, node_a_payment_amt);
+ let retry = Retry::Attempts(0);
+ nodes[0].node.send_payment(pmt_hash, onion, payment_id_a, route_params, retry).unwrap();
+ }
check_added_monitors(&nodes[0], 1);
// Node B pays 40_000 msat (the remaining part)
@@ -5577,6 +5586,12 @@ fn bolt11_multi_node_mpp() {
}
}
+#[test]
+fn bolt11_multi_node_mpp() {
+ do_bolt11_multi_node_mpp(true);
+ do_bolt11_multi_node_mpp(false);
+}
+
#[test]
fn bolt11_multi_node_mpp_with_retry() {
// Test that multi-node MPP payments work correctly when one node's initial payment attempt |
TheBlueMatt
force-pushed
the
2026-02-partial-mpp-payments
branch
from
9ceb9d6 to
a28e7e6
Compare
|
Oh, also had to update some of the new splicing tests. |
6 participants
This adds support for it both in the more manual
send_paymentflow as well aspay_for_bolt11_invoice. Adding support for BOLT 12 is left for a followup. cc @benthecarman