Skip to content
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.

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

Provide the HTLCs that settled a payment. #2478

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

waterson
Copy link
Contributor

@waterson waterson commented Aug 6, 2023

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
Copy link

codecov-commenter commented Aug 7, 2023

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 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
Copy link
Contributor Author

waterson commented Aug 8, 2023

@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
Copy link
Contributor

wpaulino commented Aug 9, 2023

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.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM after squash

@valentinewallace
Copy link
Contributor

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

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
lightning/src/events/mod.rs Outdated Show resolved Hide resolved
lightning/src/events/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

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 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
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Basically LGTM, two minor comments and one nit.

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
@TheBlueMatt
Copy link
Collaborator

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.
Comment on lines +105 to +106
}
impl_writeable_tlv_based!(ClaimedHTLC, {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@TheBlueMatt TheBlueMatt merged commit 4bd4f02 into lightningdevkit:main Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment
7 participants