Skip to content

Provide the HTLCs that settled a payment. - #2478

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
waterson:settle-htlcs
Aug 21, 2023
Merged

Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
waterson:settle-htlcs

Conversation

@waterson

@watersonwaterson commented Aug 6, 2023

Copy link
Copy Markdown
Contributor

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

codecov-commenter commented Aug 7, 2023•
edited
Loading

Copy link
Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.

wpaulino
wpaulino reviewed Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs Outdated
valentinewallace
valentinewallace reviewed Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
@waterson

waterson commented Aug 8, 2023

Copy link
Copy Markdown
ContributorAuthor

@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

wpaulino commented Aug 9, 2023

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

wpaulino
wpaulino reviewed Aug 9, 2023
View reviewed changes

@wpaulinowpaulino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM after squash

@valentinewallace

valentinewallace commented Aug 10, 2023

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

valentinewallace
valentinewallace previously approved these changes Aug 11, 2023
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Aug 13, 2023
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
vladimirfomene
vladimirfomene reviewed Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed the settle-htlcs branch 2 times, most recently from 9dd402c to 89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt reviewed Aug 18, 2023
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt

TheBlueMatt commented Aug 19, 2023

Copy link
Copy Markdown
Collaborator

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

@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen approved these changes Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Comment on lines +105 to +106
}
impl_writeable_tlv_based!(ClaimedHTLC, {

@dunxendunxenAug 21, 2023

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

TheBlueMatt
TheBlueMatt approved these changes Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
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,

@TheBlueMattTheBlueMattAug 21, 2023

Copy link
Copy Markdown
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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments
+1 more reviewer
@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment

7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning · GitHub
Skip to content
Provide the HTLCs that settled a payment. - #2478
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Aug 21, 2023
Merged
Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Conversation
@waterson
@watersonwaterson
commented
Aug 6, 2023

Copy link


Copy Markdown

Contributor
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
codecov-commenter
commented
Aug 7, 2023•

edited

Loading

Copy link


Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.
wpaulino
wpaulino
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

valentinewallace
valentinewallace
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@waterson
waterson
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
wpaulino
commented
Aug 9, 2023

Copy link


Copy Markdown

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.
wpaulino
wpaulino
reviewed
Aug 9, 2023
View reviewed changes
@wpaulinowpaulino
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM after squash
@valentinewallace
valentinewallace
commented
Aug 10, 2023

Copy link


Copy Markdown

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
valentinewallace
valentinewallace
previously approved these changes
Aug 11, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
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.
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Aug 13, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

vladimirfomene
vladimirfomene
reviewed
Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed
the
settle-htlcs
branch
2 times, most recently
from
9dd402c to
89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt
reviewed
Aug 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt
TheBlueMatt
commented
Aug 19, 2023

Copy link


Copy Markdown

Collaborator
This LGTM, feel free to squash the fixup commit, I think.
@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs

Comment on lines

+105
 to 
+106

}
impl_writeable_tlv_based!(ClaimedHTLC, {
@dunxendunxenAug 21, 2023

Copy link


Copy Markdown

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.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 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,
@TheBlueMattTheBlueMattAug 21, 2023

Copy link


Copy Markdown

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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments

+1 more reviewer

@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment


7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning · GitHub
Skip to content
Provide the HTLCs that settled a payment. - #2478
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Aug 21, 2023
Merged
Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Conversation
@waterson
@watersonwaterson
commented
Aug 6, 2023

Copy link


Copy Markdown

Contributor
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
codecov-commenter
commented
Aug 7, 2023•

edited

Loading

Copy link


Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.
wpaulino
wpaulino
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

valentinewallace
valentinewallace
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@waterson
waterson
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
wpaulino
commented
Aug 9, 2023

Copy link


Copy Markdown

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.
wpaulino
wpaulino
reviewed
Aug 9, 2023
View reviewed changes
@wpaulinowpaulino
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM after squash
@valentinewallace
valentinewallace
commented
Aug 10, 2023

Copy link


Copy Markdown

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
valentinewallace
valentinewallace
previously approved these changes
Aug 11, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
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.
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Aug 13, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

vladimirfomene
vladimirfomene
reviewed
Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed
the
settle-htlcs
branch
2 times, most recently
from
9dd402c to
89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt
reviewed
Aug 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt
TheBlueMatt
commented
Aug 19, 2023

Copy link


Copy Markdown

Collaborator
This LGTM, feel free to squash the fixup commit, I think.
@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs

Comment on lines

+105
 to 
+106

}
impl_writeable_tlv_based!(ClaimedHTLC, {
@dunxendunxenAug 21, 2023

Copy link


Copy Markdown

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.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 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,
@TheBlueMattTheBlueMattAug 21, 2023

Copy link


Copy Markdown

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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments

+1 more reviewer

@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment


7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning · GitHub
Skip to content
Provide the HTLCs that settled a payment. - #2478
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Aug 21, 2023
Merged
Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Conversation
@waterson
@watersonwaterson
commented
Aug 6, 2023

Copy link


Copy Markdown

Contributor
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
codecov-commenter
commented
Aug 7, 2023•

edited

Loading

Copy link


Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.
wpaulino
wpaulino
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

valentinewallace
valentinewallace
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@waterson
waterson
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
wpaulino
commented
Aug 9, 2023

Copy link


Copy Markdown

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.
wpaulino
wpaulino
reviewed
Aug 9, 2023
View reviewed changes
@wpaulinowpaulino
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM after squash
@valentinewallace
valentinewallace
commented
Aug 10, 2023

Copy link


Copy Markdown

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
valentinewallace
valentinewallace
previously approved these changes
Aug 11, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
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.
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Aug 13, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

vladimirfomene
vladimirfomene
reviewed
Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed
the
settle-htlcs
branch
2 times, most recently
from
9dd402c to
89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt
reviewed
Aug 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt
TheBlueMatt
commented
Aug 19, 2023

Copy link


Copy Markdown

Collaborator
This LGTM, feel free to squash the fixup commit, I think.
@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs

Comment on lines

+105
 to 
+106

}
impl_writeable_tlv_based!(ClaimedHTLC, {
@dunxendunxenAug 21, 2023

Copy link


Copy Markdown

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.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 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,
@TheBlueMattTheBlueMattAug 21, 2023

Copy link


Copy Markdown

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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments

+1 more reviewer

@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment


7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning · GitHub
Skip to content
Provide the HTLCs that settled a payment. - #2478
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Aug 21, 2023
Merged
Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Conversation
@waterson
@watersonwaterson
commented
Aug 6, 2023

Copy link


Copy Markdown

Contributor
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
codecov-commenter
commented
Aug 7, 2023•

edited

Loading

Copy link


Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.
wpaulino
wpaulino
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

valentinewallace
valentinewallace
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@waterson
waterson
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
wpaulino
commented
Aug 9, 2023

Copy link


Copy Markdown

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.
wpaulino
wpaulino
reviewed
Aug 9, 2023
View reviewed changes
@wpaulinowpaulino
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM after squash
@valentinewallace
valentinewallace
commented
Aug 10, 2023

Copy link


Copy Markdown

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
valentinewallace
valentinewallace
previously approved these changes
Aug 11, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
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.
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Aug 13, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

vladimirfomene
vladimirfomene
reviewed
Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed
the
settle-htlcs
branch
2 times, most recently
from
9dd402c to
89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt
reviewed
Aug 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt
TheBlueMatt
commented
Aug 19, 2023

Copy link


Copy Markdown

Collaborator
This LGTM, feel free to squash the fixup commit, I think.
@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs

Comment on lines

+105
 to 
+106

}
impl_writeable_tlv_based!(ClaimedHTLC, {
@dunxendunxenAug 21, 2023

Copy link


Copy Markdown

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.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 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,
@TheBlueMattTheBlueMattAug 21, 2023

Copy link


Copy Markdown

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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments

+1 more reviewer

@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment


7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning · GitHub
Skip to content
Provide the HTLCs that settled a payment. - #2478
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Aug 21, 2023
Merged
Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Conversation
@waterson
@watersonwaterson
commented
Aug 6, 2023

Copy link


Copy Markdown

Contributor
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
codecov-commenter
commented
Aug 7, 2023•

edited

Loading

Copy link


Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.
wpaulino
wpaulino
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

valentinewallace
valentinewallace
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@waterson
waterson
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
wpaulino
commented
Aug 9, 2023

Copy link


Copy Markdown

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.
wpaulino
wpaulino
reviewed
Aug 9, 2023
View reviewed changes
@wpaulinowpaulino
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM after squash
@valentinewallace
valentinewallace
commented
Aug 10, 2023

Copy link


Copy Markdown

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
valentinewallace
valentinewallace
previously approved these changes
Aug 11, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
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.
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Aug 13, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

vladimirfomene
vladimirfomene
reviewed
Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed
the
settle-htlcs
branch
2 times, most recently
from
9dd402c to
89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt
reviewed
Aug 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt
TheBlueMatt
commented
Aug 19, 2023

Copy link


Copy Markdown

Collaborator
This LGTM, feel free to squash the fixup commit, I think.
@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs

Comment on lines

+105
 to 
+106

}
impl_writeable_tlv_based!(ClaimedHTLC, {
@dunxendunxenAug 21, 2023

Copy link


Copy Markdown

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.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 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,
@TheBlueMattTheBlueMattAug 21, 2023

Copy link


Copy Markdown

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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments

+1 more reviewer

@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment


7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning · GitHub
Skip to content
Provide the HTLCs that settled a payment. - #2478
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Aug 21, 2023
Merged
Provide the HTLCs that settled a payment.#2478
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
waterson:settle-htlcs
Conversation
@waterson
@watersonwaterson
commented
Aug 6, 2023

Copy link


Copy Markdown

Contributor
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
codecov-commenter
commented
Aug 7, 2023•

edited

Loading

Copy link


Copy Markdown

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 ChangedCoverage Δ
lightning/src/events/mod.rs43.12% <66.66%> (+0.60%)⬆️
lightning/src/ln/channelmanager.rs85.55% <100.00%> (+0.03%)⬆️
lightning/src/ln/functional_test_utils.rs89.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.
wpaulino
wpaulino
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

valentinewallace
valentinewallace
reviewed
Aug 7, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@waterson
waterson
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
wpaulino
commented
Aug 9, 2023

Copy link


Copy Markdown

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.
wpaulino
wpaulino
reviewed
Aug 9, 2023
View reviewed changes
@wpaulinowpaulino
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM after squash
@valentinewallace
valentinewallace
commented
Aug 10, 2023

Copy link


Copy Markdown

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
valentinewallace
valentinewallace
previously approved these changes
Aug 11, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
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.
Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Aug 13, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

vladimirfomene
vladimirfomene
reviewed
Aug 17, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
@waterson
watersonforce-pushed
the
settle-htlcs
branch
2 times, most recently
from
9dd402c to
89808b3CompareAugust 17, 2023 21:26
TheBlueMatt
TheBlueMatt
reviewed
Aug 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
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.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Comment threadlightning/src/ln/functional_test_utils.rs
@TheBlueMatt
TheBlueMatt
commented
Aug 19, 2023

Copy link


Copy Markdown

Collaborator
This LGTM, feel free to squash the fixup commit, I think.
@waterson
Provide the HTLCs that settled a payment.
2e562a2
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.
Fixeslightningdevkit#2477.
dunxen
dunxen
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs

Comment on lines

+105
 to 
+106

}
impl_writeable_tlv_based!(ClaimedHTLC, {
@dunxendunxenAug 21, 2023

Copy link


Copy Markdown

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.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 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,
@TheBlueMattTheBlueMattAug 21, 2023

Copy link


Copy Markdown

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:mainAug 21, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
@valentinewallacevalentinewallacevalentinewallace left review comments

+1 more reviewer

@vladimirfomenevladimirfomenevladimirfomene left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Provide HTLCs that settle a claimed payment


7 participants

@waterson@codecov-commenter@wpaulino@valentinewallace@TheBlueMatt@dunxen@vladimirfomene