Provide the HTLCs that settled a payment. #2478

waterson · 2023-08-06T22:47:19Z

Creates a new events::ClaimedHTLC struct that contains the relevant information about a claimed HTLC; e.g., the channel it arrived on, its ID, the amount of the HTLC, the overall amount of the payment, etc. Adds appropriate serialization support.

Adds a Vec<events::ClaimedHTLC> to the ClaimingPayment structure. Populates this when creating the struct by converting the payment.htlcs (which are ClaimingHTLC structs) into event::ClaimedHTLC structs. This is a straightforward transformation.

Adds a Vec<events::ClaimedHTLC> to the events::Event::PaymentClaimed enum. This is populated directly from the ClaimingPayment's htlcs vec.

Fixes #2477.

codecov-commenter · 2023-08-07T00:08:38Z

Codecov Report

Patch coverage: 94.54% and project coverage change: -0.02% ⚠️

Comparison is base (d4ad826) 90.40% compared to head (ec1bd27) 90.39%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
- Coverage   90.40%   90.39%   -0.02%     
==========================================
  Files         106      106              
  Lines       56268    56320      +52     
  Branches    56268    56320      +52     
==========================================
+ Hits        50868    50908      +40     
- Misses       5400     5412      +12

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`43.12% <66.66%> (+0.60%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.55% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`89.00% <100.00%> (+0.17%)`	⬆️

... and 3 files with indirect coverage changes

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

lightning/src/events/mod.rs

waterson · 2023-08-08T13:22:19Z

@valentinewallace @wpaulino thank you for the reviews!

I went ahead and promoted sender_intended_total_msat from the ClaimedHTLC to the PaymentClaimed struct. That said, I'm not sure I understand when that value would be different from PaymentClaimed.amount_msat?

wpaulino · 2023-08-09T22:22:24Z

I went ahead and promoted sender_intended_total_msat from the ClaimedHTLC to the PaymentClaimed struct. That said, I'm not sure I understand when that value would be different from PaymentClaimed.amount_msat?

This is probably due to #2319.

wpaulino

LGTM after squash

valentinewallace · 2023-08-10T20:22:57Z

I went ahead and promoted sender_intended_total_msat from the ClaimedHTLC to the PaymentClaimed struct. That said, I'm not sure I understand when that value would be different from PaymentClaimed.amount_msat?

This is probably due to #2319.

This is due to #2062, see PR description for the reasoning behind this value. We probably want to add some more documentation to sender_intended_total with some of this info as well

valentinewallace

No blocking feedback!

For test coverage, could we add this diff to functional_test_utils:

diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 39396685f..438dc2f90 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -2253,12 +2253,16 @@ pub fn do_claim_payment_along_route_with_extra_penultimate_hop_fees<'a, 'b, 'c>(

        let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events();
        assert_eq!(claim_event.len(), 1);
-       match claim_event[0] {
-               Event::PaymentClaimed { purpose: PaymentPurpose::SpontaneousPayment(preimage), .. }|
-               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { payment_preimage: Some(preimage), ..}, .. } =>
-                       assert_eq!(preimage, our_payment_preimage),
-               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { .. }, payment_hash, .. } =>
-                       assert_eq!(&payment_hash.0, &Sha256::hash(&our_payment_preimage.0)[..]),
+       match &claim_event[0] {
+               Event::PaymentClaimed { purpose: PaymentPurpose::SpontaneousPayment(preimage), htlcs, .. } |
+               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { payment_preimage: Some(preimage), ..}, htlcs, .. } => {
+                       assert_eq!(htlcs.len(), expected_paths.len());
+                       assert_eq!(preimage, &our_payment_preimage);
+               },
+               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { .. }, payment_hash, htlcs, .. } => {
+                       assert_eq!(htlcs.len(), expected_paths.len());
+                       assert_eq!(&payment_hash.0, &Sha256::hash(&our_payment_preimage.0)[..]);
+               },
                _ => panic!(),
        }

And also modify an existing (or new) test call to expect_payment_claimed! to check each ClaimedHTLC field explicitly.

lightning/src/events/mod.rs

TheBlueMatt

Please squash the fixup commit - in general we shouldn't land PRs that have commits in them that fix bugs in the previous commits in the same PR. When you do so, please also line-break the first commit in the PR at 70 chars.

lightning/src/events/mod.rs

TheBlueMatt

Basically LGTM, two minor comments and one nit.

lightning/src/events/mod.rs

lightning/src/ln/functional_test_utils.rs

TheBlueMatt · 2023-08-19T02:42:24Z

This LGTM, feel free to squash the fixup commit, I think.

Creates a new `events::ClaimedHTLC` struct that contains the relevant information about a claimed HTLC; e.g., the channel it arrived on, its ID, the amount of the HTLC, the overall amount of the payment, etc. Adds appropriate serialization support. Adds a `Vec<events::ClaimedHTLC>` to the `ClaimingPayment` structure. Populates this when creating the struct by converting the `payment.htlcs` (which are `ClaimingHTLC` structs) into `event::ClaimedHTLC` structs. This is a straightforward transformation. Adds a `Vec<events::ClaimedHTLC>` to the `events::Event::PaymentClaimed` enum. This is populated directly from the `ClaimingPayment`'s `htlcs` vec. Fixes lightningdevkit#2477.

dunxen · 2023-08-21T18:11:33Z

lightning/src/events/mod.rs

+}
+impl_writeable_tlv_based!(ClaimedHTLC, {


nit: separate with newline, but so nitty feel free to ignore.

TheBlueMatt · 2023-08-21T22:24:29Z

lightning/src/ln/channelmanager.rs

@@ -5129,9 +5152,20 @@ where
 			match action {
 				MonitorUpdateCompletionAction::PaymentClaimed { payment_hash } => {
 					let payment = self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
-					if let Some(ClaimingPayment { amount_msat, payment_purpose: purpose, receiver_node_id }) = payment {
+					if let Some(ClaimingPayment {
+						amount_msat,


At
least
personally,
I
find
the
rustfmt
comically-vertical
flows
to
be
kinda
hard
to
read
:)

wpaulino reviewed Aug 7, 2023

View reviewed changes

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

valentinewallace reviewed Aug 7, 2023

View reviewed changes

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

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

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

waterson requested review from wpaulino and valentinewallace August 8, 2023 12:54

wpaulino reviewed Aug 9, 2023

View reviewed changes

valentinewallace previously approved these changes Aug 11, 2023

View reviewed changes

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

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

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

TheBlueMatt reviewed Aug 13, 2023

View reviewed changes

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

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

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

vladimirfomene reviewed Aug 17, 2023

View reviewed changes

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

waterson dismissed valentinewallace’s stale review via 9dd402c August 17, 2023 21:22

waterson force-pushed the settle-htlcs branch 2 times, most recently from 9dd402c to 89808b3 Compare August 17, 2023 21:26

TheBlueMatt reviewed Aug 18, 2023

View reviewed changes

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

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

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

waterson force-pushed the settle-htlcs branch from ec1bd27 to 2e562a2 Compare August 21, 2023 15:30

dunxen approved these changes Aug 21, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 21, 2023

View reviewed changes

TheBlueMatt merged commit 4bd4f02 into lightningdevkit:main Aug 21, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide the HTLCs that settled a payment. #2478

Provide the HTLCs that settled a payment. #2478

waterson commented Aug 6, 2023

codecov-commenter commented Aug 7, 2023 •

edited

waterson commented Aug 8, 2023

wpaulino commented Aug 9, 2023

wpaulino left a comment

valentinewallace commented Aug 10, 2023

valentinewallace left a comment

TheBlueMatt left a comment

TheBlueMatt left a comment

TheBlueMatt commented Aug 19, 2023

dunxen Aug 21, 2023

TheBlueMatt Aug 21, 2023

		}
		impl_writeable_tlv_based!(ClaimedHTLC, {

Provide the HTLCs that settled a payment. #2478

Provide the HTLCs that settled a payment. #2478

Conversation

waterson commented Aug 6, 2023

codecov-commenter commented Aug 7, 2023 • edited

Codecov Report

waterson commented Aug 8, 2023

wpaulino commented Aug 9, 2023

wpaulino left a comment

Choose a reason for hiding this comment

valentinewallace commented Aug 10, 2023

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 19, 2023

dunxen Aug 21, 2023

Choose a reason for hiding this comment

TheBlueMatt Aug 21, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 7, 2023 •

edited