Skip to content

Expand PaymentClaimable to include all inbound channel IDs for a payment #3655

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 3 commits into
base: main
Choose a base branch
from
+100 −37
Open

Expand PaymentClaimable to include all inbound channel IDs for a payment #3655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e61f68b
Introduce ClaimablePayment utilities
shaavan Apr 4, 2025
d85a29d
Update `PaymentClaimable` fields to improve MPP clarity
shaavan Mar 8, 2025
477393c
Expand Test suite
shaavan Apr 21, 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
42 changes: 29 additions & 13 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,10 +774,10 @@ pub enum Event {
/// Information for claiming this received payment, based on whether the purpose of the
/// payment is to pay an invoice or to send a spontaneous payment.
purpose: PaymentPurpose,
/// The `channel_id` indicating over which channel we received the payment.
via_channel_id: Option<ChannelId>,
/// The `user_channel_id` indicating over which channel we received the payment.
via_user_channel_id: Option<u128>,
/// The `(channel_id, user_channel_id)` pairs over which the payment was received.
///
/// This will be an incomplete vector for MPP payment events created/serialized using LDK version 0.1.0 and prior.
inbound_channel_ids: Vec<(ChannelId, Option<u128>)>,
/// The block height at which this payment will be failed back and will no longer be
/// eligible for claiming.
///
Expand Down Expand Up @@ -1506,7 +1506,7 @@ impl Writeable for Event {
// drop any channels which have not yet exchanged funding_signed.
},
&Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat,
ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id,
ref purpose, ref receiver_node_id, ref inbound_channel_ids,
ref claim_deadline, ref onion_fields, ref payment_id,
} => {
1u8.write(writer)?;
Expand Down Expand Up @@ -1540,20 +1540,30 @@ impl Writeable for Event {
}
let skimmed_fee_opt = if counterparty_skimmed_fee_msat == 0 { None }
else { Some(counterparty_skimmed_fee_msat) };

let (via_channel_id_legacy, via_user_channel_id_legacy) = match inbound_channel_ids.last() {
Some((chan_id, user_chan_id)) => (Some(*chan_id), *user_chan_id),
None => (None, None),
};
write_tlv_fields!(writer, {
(0, payment_hash, required),
(1, receiver_node_id, option),
(2, payment_secret, option),
(3, via_channel_id, option),
// Marked as legacy in version 0.2.0; superseded by `inbound_channel_ids`, which
// includes all channel IDs used in the payment instead of only the last one.
(3, via_channel_id_legacy, option),
(4, amount_msat, required),
(5, via_user_channel_id, option),
// Marked as legacy in version 0.2.0 for the same reason as `via_channel_id_legacy`;
// superseded by `via_user_channel_ids`.
(5, via_user_channel_id_legacy, option),
// Type 6 was `user_payment_id` on 0.0.103 and earlier
(7, claim_deadline, option),
(8, payment_preimage, option),
(9, onion_fields, option),
(10, skimmed_fee_opt, option),
(11, payment_context, option),
(13, payment_id, option),
(15, *inbound_channel_ids, optional_vec),
});
},
&Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref amount_msat, ref fee_paid_msat } => {
Expand Down Expand Up @@ -1849,41 +1859,47 @@ impl MaybeReadable for Event {
let mut counterparty_skimmed_fee_msat_opt = None;
let mut receiver_node_id = None;
let mut _user_payment_id = None::<u64>; // Used in 0.0.103 and earlier, no longer written in 0.0.116+.
let mut via_channel_id = None;
let mut via_channel_id_legacy = None;
let mut claim_deadline = None;
let mut via_user_channel_id = None;
let mut via_user_channel_id_legacy = None;
let mut onion_fields = None;
let mut payment_context = None;
let mut payment_id = None;
let mut inbound_channel_ids_opt = None;
read_tlv_fields!(reader, {
(0, payment_hash, required),
(1, receiver_node_id, option),
(2, payment_secret, option),
(3, via_channel_id, option),
(3, via_channel_id_legacy, option),
(4, amount_msat, required),
(5, via_user_channel_id, option),
(5, via_user_channel_id_legacy, option),
(6, _user_payment_id, option),
(7, claim_deadline, option),
(8, payment_preimage, option),
(9, onion_fields, option),
(10, counterparty_skimmed_fee_msat_opt, option),
(11, payment_context, option),
(13, payment_id, option),
(15, inbound_channel_ids_opt, optional_vec),
});
let purpose = match payment_secret {
Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context)
.map_err(|()| msgs::DecodeError::InvalidValue)?,
None if payment_preimage.is_some() => PaymentPurpose::SpontaneousPayment(payment_preimage.unwrap()),
None => return Err(msgs::DecodeError::InvalidValue),
};

let inbound_channel_ids = inbound_channel_ids_opt
.or_else(|| via_channel_id_legacy.map(|chan_id| vec![(chan_id, via_user_channel_id_legacy)]))
.unwrap_or_default();

Ok(Some(Event::PaymentClaimable {
receiver_node_id,
payment_hash,
amount_msat,
counterparty_skimmed_fee_msat: counterparty_skimmed_fee_msat_opt.unwrap_or(0),
purpose,
via_channel_id,
via_user_channel_id,
inbound_channel_ids,
claim_deadline,
onion_fields,
payment_id,
Expand Down
20 changes: 19 additions & 1 deletion lightning/src/ln/blinded_payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,26 @@ fn mpp_to_one_hop_blinded_path() {
Some(payment_secret), ev.clone(), false, None);

let ev = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
let event = pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
Some(payment_secret), ev.clone(), true, None);

match event.unwrap() {
Event::PaymentClaimable { mut inbound_channel_ids, .. } => {
let mut expected_inbound_channel_ids = nodes[3].node.list_channels()
.iter()
.map(|d| (d.channel_id, Some(d.user_channel_id)))
.collect::<Vec<(_, _)>>();

// `list_channels` returns channels in arbitrary order, so we sort both vectors
// to ensure the comparison is order-agnostic.
inbound_channel_ids.sort();
expected_inbound_channel_ids.sort();

assert_eq!(inbound_channel_ids, expected_inbound_channel_ids);
}
_ => panic!("Unexpected event"),
}

claim_payment_along_route(
ClaimAlongRouteArgs::new(&nodes[0], expected_route, payment_preimage)
);
Expand Down
27 changes: 16 additions & 11 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;

let (route, payment_hash_1, payment_preimage_1, payment_secret_1) = get_route_and_payment_hash!(&nodes[0], nodes[1], 1000000);

Expand Down Expand Up @@ -165,11 +166,11 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
let events_3 = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events_3.len(), 1);
match events_3[0] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(payment_hash_1, *payment_hash);
assert_eq!(amount_msat, 1_000_000);
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
assert_eq!(via_channel_id, Some(channel_id));
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand Down Expand Up @@ -247,6 +248,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;

let (payment_preimage_1, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);

Expand Down Expand Up @@ -547,11 +549,12 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
let events_5 = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events_5.len(), 1);
match events_5[0] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(payment_hash_2, *payment_hash);
assert_eq!(amount_msat, 1_000_000);
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
assert_eq!(via_channel_id, Some(channel_id));
assert_eq!(inbound_channel_ids.len(), 1);
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand Down Expand Up @@ -601,6 +604,7 @@ fn test_monitor_update_fail_cs() {
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;

let (route, our_payment_hash, payment_preimage, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000);
{
Expand Down Expand Up @@ -665,11 +669,11 @@ fn test_monitor_update_fail_cs() {
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(payment_hash, our_payment_hash);
assert_eq!(amount_msat, 1_000_000);
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
assert_eq!(via_channel_id, Some(channel_id));
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand Down Expand Up @@ -1678,12 +1682,12 @@ fn test_monitor_update_fail_claim() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 2);
match events[0] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(payment_hash_2, *payment_hash);
assert_eq!(1_000_000, amount_msat);
assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
assert_eq!(via_channel_id, Some(channel_id));
assert_eq!(via_user_channel_id, Some(42));
assert_eq!(inbound_channel_ids.len(), 1);
assert_eq!(*inbound_channel_ids.last().unwrap(), (channel_id, Some(42)));
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand All @@ -1695,11 +1699,12 @@ fn test_monitor_update_fail_claim() {
_ => panic!("Unexpected event"),
}
match events[1] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(payment_hash_3, *payment_hash);
assert_eq!(1_000_000, amount_msat);
assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
assert_eq!(via_channel_id, Some(channel_id));
assert_eq!(inbound_channel_ids.len(), 1);
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(42))]);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand Down
25 changes: 22 additions & 3 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,15 @@ impl ClaimablePayment {
self.htlcs.iter().map(|htlc| (htlc.prev_hop.channel_id, htlc.prev_hop.htlc_id))
)
}

/// Returns the inbound `(channel_id, user_channel_id)` pairs for all HTLCs associated with the payment.
///
/// Note: The `user_channel_id` will be `None` for HTLCs created using LDK version 0.0.117 or prior.
fn inbound_channel_ids(&self) -> Vec<(ChannelId, Option<u128>)> {
self.htlcs.iter().map(|htlc| {
(htlc.prev_hop.channel_id, htlc.prev_hop.user_channel_id)
}).collect()
}
}

/// Represent the channel funding transaction type.
Expand Down Expand Up @@ -6282,8 +6291,7 @@ where
purpose: $purpose,
amount_msat,
counterparty_skimmed_fee_msat,
via_channel_id: Some(prev_channel_id),
via_user_channel_id: Some(prev_user_channel_id),
inbound_channel_ids: claimable_payment.inbound_channel_ids(),
claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER),
onion_fields: claimable_payment.onion_fields.clone(),
payment_id: Some(payment_id),
Expand Down Expand Up @@ -10917,7 +10925,18 @@ where
let events = core::cell::RefCell::new(Vec::new());
let event_handler = |event: events::Event| Ok(events.borrow_mut().push(event));
self.process_pending_events(&event_handler);
events.into_inner()
let collected_events = events.into_inner();

// To expand the coverage and make sure all events are properly serialised and deserialised,
// we test all generated events round-trip:
for event in &collected_events {
let ser = event.encode();
if let Some(deser) = events::Event::read(&mut &ser[..]).expect("event should deserialize") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the None case ok to ignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIU, yes. Some event enums are designed to map to a None value (for example, 0u8, 4u8, and 17u8), so getting an Ok(None) seems to indicate that the event was read successfully and no further checks are needed in this case.

That said, I'd love to hear @valentinewallace's thoughts to confirm if I’m understanding this correctly!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, some events are never written so I believe @shaavan is correct here

Copy link
Contributor

Choose a reason for hiding this comment

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

So could also continue when ser is None then, and unwrap the read? Not that it is all that important ofc.

Copy link
Contributor

@valentinewallace valentinewallace Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ser just returns Ok(()) on success, not an option.. But if you want to rearrange this code somehow, no strong preference here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like ser is just Vec<u8>. So if that is empty, we expect None from read? Maybe that can be used to make it slightly stricter and detect the case where read is unexpectedly returning None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, sorry. It looks like for events where we don't write the actual event, we will still write the type for the event: https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/events/mod.rs#L1809 so I don't think we can check if it's empty but we could check if only a u8 was written.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a too much internal detail then. Can leave this as is as far as I am concerned.

assert_eq!(&deser, event, "event should roundtrip correctly");
}
}

collected_events
}

#[cfg(feature = "_test_utils")]
Expand Down
10 changes: 7 additions & 3 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2734,7 +2734,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
assert_eq!(events_2.len(), 1);
match &events_2[0] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat,
receiver_node_id, ref via_channel_id, ref via_user_channel_id,
receiver_node_id, ref inbound_channel_ids,
claim_deadline, onion_fields, ..
} => {
assert_eq!(our_payment_hash, *payment_hash);
Expand Down Expand Up @@ -2768,8 +2768,12 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
},
}
assert_eq!(*amount_msat, recv_value);
assert!(node.node.list_channels().iter().any(|details| details.channel_id == via_channel_id.unwrap()));
assert!(node.node.list_channels().iter().any(|details| details.user_channel_id == via_user_channel_id.unwrap()));
for (chan_id, user_chan_id) in inbound_channel_ids {
assert!(node.node.list_channels().iter().any(|details| &details.channel_id == chan_id));
if let Some(user_id) = user_chan_id {
assert!(node.node.list_channels().iter().any(|details| &details.user_channel_id == user_id));
}
}
assert!(claim_deadline.unwrap() > node.best_block_info().1);
},
_ => panic!("Unexpected event"),
Expand Down
13 changes: 7 additions & 6 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2068,11 +2068,11 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
let events = nodes[2].node.get_and_clear_pending_events();
assert_eq!(events.len(), 2);
match events[0] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(our_payment_hash_21, *payment_hash);
assert_eq!(recv_value_21, amount_msat);
assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
assert_eq!(via_channel_id, Some(chan_2.2));
assert_eq!(inbound_channel_ids.last().unwrap().0, chan_2.2);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand All @@ -2084,11 +2084,11 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
_ => panic!("Unexpected event"),
}
match events[1] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(our_payment_hash_22, *payment_hash);
assert_eq!(recv_value_22, amount_msat);
assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
assert_eq!(via_channel_id, Some(chan_2.2));
assert_eq!(inbound_channel_ids.last().unwrap().0, chan_2.2);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand Down Expand Up @@ -4313,11 +4313,12 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
let events_2 = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events_2.len(), 1);
match events_2[0] {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
assert_eq!(payment_hash_1, *payment_hash);
assert_eq!(amount_msat, 1_000_000);
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
assert_eq!(via_channel_id, Some(channel_id));
assert_eq!(inbound_channel_ids.len(), 1);
assert_eq!(inbound_channel_ids.last().unwrap().0, channel_id);
match &purpose {
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
assert!(payment_preimage.is_none());
Expand Down
Loading