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

shaavan · 2025-03-08T13:16:12Z

Resolves #2274

This PR expands PaymentClaimable to include all inbound channel_ids and user_channel_ids associated with a payment.
This update ensures a clearer representation of the channels used, especially for MPP payments.

ldk-reviews-bot · 2025-03-08T13:16:14Z

👋 Thanks for assigning @joostjager 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.

shaavan · 2025-03-08T13:24:16Z

@benthecarman

Would also be nice if the same information is available on the PaymentClaimed event too

In the current version, I haven’t included channel_ids and user_channel_ids for PaymentClaimed, as they can be easily derived from the event itself:

let channel_ids = payment_claimed_event.htlcs.iter().map(|htlc| htlc.channel_id).collect();

That said, let me know if you’d still prefer to include channel_ids in PaymentClaimed, and I'd be happy to make that change. Thanks!

benthecarman · 2025-03-08T19:19:19Z

Thanks, @shaavan , that is good enough

lightning/src/ln/functional_test_utils.rs

lightning/src/events/mod.rs

ldk-reviews-bot · 2025-03-10T16:21:10Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

valentinewallace · 2025-03-10T16:23:09Z

nit: limit commit subject line in 2nd commit to ~50 characters

shaavan · 2025-03-12T18:10:53Z

Updated from pr3655.01 to pr3655.02 (diff):
Addressed @valentinewallace comments

Updated the fields to be Vec<ChannelId>, and Vec<u128>
Updated test utilities to check for all the channels id in events list, for improved test completeness.

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-03-12T19:29:13Z

lightning/src/events/mod.rs

+					// legacy field
+					// (3, via_channel_id, option),
 					(4, amount_msat, required),
-					(5, via_user_channel_id, option),
+					// legacy field
+					// (5, via_user_channel_id, option),


I think we should still write these fields for the sake of users who downgrade (can always write the first item in the vec). Also would be nice to comment in what version the fields became legacy.

Done! pr3655.03
I’ve updated the writing so that we maintain downgrade support.
Haven’t added a comment yet since I’m not sure what the next version number will be 😅 (would it be 0.1.3?)

lightning/src/events/mod.rs

shaavan · 2025-03-13T18:06:48Z

Updated from pr3655.02 to pr3655.03 (diff):
Addressed @valentinewallace comments

Changes:

Expanded Documentation
Properly write the legacy fields to maintain downgrade support.

valentinewallace · 2025-03-13T18:33:18Z

Looks good, feel free to squash! Thanks!

shaavan · 2025-03-15T07:27:21Z

Updated from pr3655.03 to pr3655.04 (diff):
Addressed @valentinewallace comments

Changes:

Squash commits.

codecov · 2025-03-15T07:35:13Z

Codecov Report

Attention: Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.37%. Comparing base (5bc9ffa) to head (591c5c8).
Report is 180 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/mod.rs	90.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3655      +/-   ##
==========================================
+ Coverage   89.21%   91.37%   +2.16%     
==========================================
  Files         155      156       +1     
  Lines      118966   141344   +22378     
  Branches   118966   141344   +22378     
==========================================
+ Hits       106133   129159   +23026     
+ Misses      10253     9799     -454     
+ Partials     2580     2386     -194

☔ 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.

shaavan

Looks good, feel free to squash! Thanks!

Awesome — thanks a lot for the review, Val! 🚀

lightning/src/events/mod.rs

shaavan · 2025-03-25T13:17:09Z

Updated from pr3655.04 to pr3655.05 (diff):
Addressed @valentinewallace comments

Changes:

Added comment documenting when, and why via_channel_id_legacy, and via_user_channel_id_legacy were deprecated.

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

shaavan · 2025-04-04T13:22:05Z

Updated from pr3655.05 to pr3655.06 (diff):
Addressed @valentinewallace comments

Changes:

Updated documentation to correctly reflect that the vector will be incomplete for HTLC created using versions prior to LDK 0.1.0 or prior.

ldk-reviews-bot · 2025-04-07T00:00:09Z

🔔 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.

valentinewallace

Should be good to land after these last tweaks!

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

These two utilities will be used in following commit to get all the inbound channel_ids, and user_channel_ids associated with all the parts of a payment.

shaavan · 2025-04-08T11:14:22Z

Updated from pr3655.06 to pr3655.07 (diff):
Addressed @valentinewallace comments

Changes:

Fixed comments to correctly represent for which version and cases the lists would be incomplete.

joostjager

Code looks good, test coverage could probably be improved a bit.

lightning/src/events/mod.rs

lightning/src/ln/functional_tests.rs

shaavan · 2025-04-21T14:56:22Z

Updated from pr3655.07 to pr3655.08 (diff):
Addressed @joostjager comments

Changes:

Expanded event test-suite, to check that all events are correctly serialised-deserialised.

shaavan · 2025-04-22T13:45:04Z

Updated from pr3655.08 to pr3655.09 (diff):
Addressed @joostjager comments

Changes:

Expanded test, to ensure that all the via_channel_id & via_user_channel_ids are present in PaymentClaimable in multi part payment case.

valentinewallace · 2025-04-22T20:33:31Z

LGTM

ldk-reviews-bot · 2025-04-24T20:34:32Z

🔔 1st Reminder

Hey @joostjager! 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.

ldk-reviews-bot · 2025-04-28T00:00:13Z

🔔 2nd Reminder

Hey @joostjager! 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.

joostjager · 2025-04-25T21:54:13Z

lightning/src/events/mod.rs

+		via_channel_ids: Vec<ChannelId>,
+		/// The `user_channel_id`(s) corresponding to the channels 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.
+		via_user_channel_ids: Vec<u128>,


It's a bit late now, but would it be better to keep id and user_id together in a tuple? Not sure if there are interpretation issues when both vecs have different lengths?

Thanks for bringing this up, Joost! I did consider that, but I wasn’t sure if it would offer a clear advantage. As far as I understand, both lists will always be of equal length since both fields were introduced together in the same version, so mismatched lengths shouldn’t arise.

Also, keeping them separate makes their distinction more explicit and follows the structure we’ve used in earlier LDK versions, where via_channel_id and via_user_channel_id were also separate fields.

That said, I’m open to discussing it further if you think a tuple-based design would still be better here! Would love to hear your thoughts.

I suppose the tuple leaves less room for questions like these. And maybe the original fields should have been a tuple/struct too. In general, I think it is good to keep the data types as strict as they can be.

But here it doesn't seem to be particular critical. Happy to go along with the other reviewer's view on it.

I think we'd have to have a Vec<(ChannelId, Option<u128>)> since per ClaimablePayment::get_user_channel_ids docs user channel IDs are only always set after 117... Don't have a strong preference here although the option makes the field's API a bit less nice IMO.

The option would be consistent with HTLCPreviousHopData for example

Also, isn't it the case that pre-117 data just translates to an empty vec<(..,..)> without requiring the option?

lightning/src/ln/chanmon_update_fail_tests.rs

joostjager · 2025-04-28T09:25:01Z

lightning/src/ln/channelmanager.rs

+		// 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") {


Is the None case ok to ignore?

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!

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

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

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

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.

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.

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

Previously, `channel_id` in `PaymentClaimable` only listed a single inbound channel, which was misleading for MPP payments arriving via multiple channels. To better represent MPP scenarios, this update introduces: - `via_channel_ids`: A list of all inbound channels used in the payment. - `via_user_channel_ids`: The corresponding user-defined channel IDs for each inbound channel. This change ensures a more accurate representation of multi-path payments while maintaining backward compatibility.

- Expand testing to ensure proper serialise-deserialise round-trip for all events. - Expand a multi-Part payment test to ensure that all the channel ids & user channel ids are present in the PaymentClaimable event

shaavan · 2025-04-28T13:09:59Z

Updated from pr3655.09 to pr3655.10 (diff):
Addressed @joostjager comments

Changes:

Add check that the via_channel_ids, and via_user_channel_ids only contains one element for the appropriate cases.

joostjager · 2025-04-28T19:13:25Z

lightning/src/ln/channelmanager.rs

+
+	/// Returns the inbound `user_channel_id`s for all HTLCs associated with the payment.
+	///
+	/// Note: This list will be incomplete for HTLCs created using LDK version 0.0.117 or prior.


This comment does seem to suggest that vecs can be different lengths?

@shaavan, @valentinewallace and I chatted about it. Seems something to say for a stronger type, especially because the vecs both map from PrevHopData already. But it's a bit late now also, and not too critical. So leaving up to you to decide.

ldk-reviews-bot requested a review from valentinewallace March 8, 2025 13:27

valentinewallace reviewed Mar 10, 2025

View reviewed changes

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

shaavan force-pushed the i2274 branch from 64cea7a to 5885a54 Compare March 12, 2025 18:10

valentinewallace reviewed Mar 12, 2025

View reviewed changes

shaavan force-pushed the i2274 branch from 5885a54 to e39ccaa Compare March 13, 2025 18:06

shaavan force-pushed the i2274 branch from e39ccaa to c08d954 Compare March 15, 2025 07:27

shaavan commented Mar 15, 2025

View reviewed changes

lightning/src/events/mod.rs Show resolved Hide resolved

shaavan force-pushed the i2274 branch from c08d954 to fa90cc6 Compare March 25, 2025 13:17

valentinewallace reviewed Mar 25, 2025

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

shaavan force-pushed the i2274 branch from fa90cc6 to 591c5c8 Compare April 4, 2025 13:22

valentinewallace self-requested a review April 4, 2025 17:11

valentinewallace reviewed Apr 7, 2025

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

Introduce ClaimablePayment utilities

fe823a9

These two utilities will be used in following commit to get all the inbound channel_ids, and user_channel_ids associated with all the parts of a payment.

shaavan force-pushed the i2274 branch from 591c5c8 to 4bfd89e Compare April 8, 2025 11:14

valentinewallace previously approved these changes Apr 8, 2025

View reviewed changes

jkczyz requested a review from joostjager April 10, 2025 17:29

joostjager reviewed Apr 11, 2025

View reviewed changes

lightning/src/events/mod.rs Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Show resolved Hide resolved

shaavan dismissed valentinewallace’s stale review via 16002cd April 21, 2025 14:56

shaavan force-pushed the i2274 branch from 16002cd to b9e52f5 Compare April 22, 2025 13:45

valentinewallace requested a review from joostjager April 22, 2025 20:33

joostjager reviewed Apr 28, 2025

View reviewed changes

shaavan added 2 commits April 28, 2025 18:26

Expand Test suite

de8a8e5

- Expand testing to ensure proper serialise-deserialise round-trip for all events. - Expand a multi-Part payment test to ensure that all the channel ids & user channel ids are present in the PaymentClaimable event

shaavan force-pushed the i2274 branch from b9e52f5 to de8a8e5 Compare April 28, 2025 13:09

joostjager reviewed Apr 28, 2025

View reviewed changes

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

Are you sure you want to change the base?

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

Conversation

shaavan commented Mar 8, 2025

ldk-reviews-bot commented Mar 8, 2025 • edited Loading

shaavan commented Mar 8, 2025

benthecarman commented Mar 8, 2025

ldk-reviews-bot commented Mar 10, 2025

valentinewallace commented Mar 10, 2025

shaavan commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan commented Mar 13, 2025

valentinewallace commented Mar 13, 2025

shaavan commented Mar 15, 2025

codecov bot commented Mar 15, 2025 • edited Loading

Codecov Report

shaavan left a comment

Choose a reason for hiding this comment

shaavan commented Mar 25, 2025

shaavan commented Apr 4, 2025

ldk-reviews-bot commented Apr 7, 2025

valentinewallace left a comment

Choose a reason for hiding this comment

shaavan commented Apr 8, 2025

joostjager left a comment

Choose a reason for hiding this comment

shaavan commented Apr 21, 2025

shaavan commented Apr 22, 2025

valentinewallace commented Apr 22, 2025

ldk-reviews-bot commented Apr 24, 2025

ldk-reviews-bot commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan commented Apr 28, 2025

Choose a reason for hiding this comment

joostjager Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

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

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

ldk-reviews-bot commented Mar 8, 2025 •

edited

Loading

codecov bot commented Mar 15, 2025 •

edited

Loading

valentinewallace Apr 28, 2025 •

edited

Loading

joostjager Apr 28, 2025 •

edited

Loading