Skip to content

Log payment hash of the HTLC that causes a force closure #4003

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

Merged
merged 2 commits into from
Aug 13, 2025

Conversation

yellowred
Copy link
Contributor

When LDK force closes a channel because of an HTLC time out, we would like to know the payment hash of the HTLC so we can debug the reason for time out.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Aug 11, 2025

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

@TheBlueMatt
Copy link
Collaborator

Rather than (only) logging it, can we pipe it back through to the ClosureReason::HTLCsTimedOut to be included in the ChannelClosed event? That would make it programmatically accessible, which it seems would be nice for assigning blame too.

Copy link

codecov bot commented Aug 11, 2025

Codecov Report

❌ Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.85%. Comparing base (b39d32c) to head (e77c83d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/events/mod.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4003   +/-   ##
=======================================
  Coverage   88.84%   88.85%           
=======================================
  Files         175      175           
  Lines      127682   127710   +28     
  Branches   127682   127710   +28     
=======================================
+ Hits       113440   113472   +32     
+ Misses      11681    11674    -7     
- Partials     2561     2564    +3     
Flag Coverage Δ
fuzzing 21.86% <30.00%> (-0.01%) ⬇️
tests 88.68% <97.91%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yellowred
Copy link
Contributor Author

yellowred commented Aug 11, 2025

Rather than (only) logging it, can we pipe it back through to the ClosureReason::HTLCsTimedOut to be included in the ChannelClosed event? That would make it programmatically accessible, which it seems would be nice for assigning blame too.

Added PaymentHash to the enum variant and tried to fix the tests, but there are seems to be some tests that do not have payment hash to assert on. I commented them out, but would be glad if someone could assist with fixing them.

@yellowred yellowred force-pushed the add_htlc_payment_hash_log branch from 1b41664 to 5687ac9 Compare August 11, 2025 19:32
When LDK force closes a channel because of an HTLC time out, we
would like to know the payment hash of the HTLC so we can debug
the reason for time out.

Co-authored-by: Matt Corallo <[email protected]>
@TheBlueMatt TheBlueMatt force-pushed the add_htlc_payment_hash_log branch from 5687ac9 to b1cff7a Compare August 13, 2025 13:34
@TheBlueMatt
Copy link
Collaborator

Fixed the tests and addressed pending feedback.

When an HTLC timing out causes a channel to force-close, its useful
to have the payment hash available in a programatic way so that it
is always available for debugging. Thus, here, it is added to
`ClosureReason::HTLCsTimedOut` for inclusion in
`Event::ChanelClosed`.

Co-authored-by: Matt Corallo <[email protected]>
@TheBlueMatt TheBlueMatt force-pushed the add_htlc_payment_hash_log branch from b1cff7a to e77c83d Compare August 13, 2025 13:36
@TheBlueMatt TheBlueMatt requested review from tnull and removed request for joostjager August 13, 2025 13:36
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, happy to land once CI let's us.

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

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

@TheBlueMatt TheBlueMatt merged commit df9232b into lightningdevkit:main Aug 13, 2025
24 of 25 checks passed
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.

4 participants