rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Split out `receive_htlcs` from the forwarding pipeline

Open tnull opened this issue 4 months ago • 6 comments

This is another preparatory step for receiver-side delays that we split out to err on the side of smaller, more reviewable PRs, especially since these changes are a nice cleanup in any case, IMO.

Previously, we'd store receiving HTLCs side-by-side with HTLCs forwards in the forwards_htlcs map under SCID 0. Here, we opt to split out a separate receive_htlcs field which cleans up the logic, also omitting the 0 magic value.

Moreover, some of the tests manipulating forward_htlcs were previously just iterating, but not actually checking whether the expected entries were present and they were actually changed. Here we make these test cases stricter to ensure they'd able to catch any unwanted behavior we'd introduce while introducing receive_htlcs.

tnull avatar Jul 30 '25 12:07 tnull

👋 Thanks for assigning @TheBlueMatt as a reviewer! I'll wait for their review and will help manage the review process. Once they submit their review, I'll check if a second reviewer would be helpful.

ldk-reviews-bot avatar Jul 30 '25 12:07 ldk-reviews-bot

Codecov Report

:x: Patch coverage is 79.82456% with 23 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 88.61%. Comparing base (96f9242) to head (e08d3dc).

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 78.43% 10 Missing and 1 partial :warning:
lightning/src/ln/onion_route_tests.rs 79.48% 8 Missing :warning:
lightning/src/ln/payment_tests.rs 83.33% 4 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3973      +/-   ##
==========================================
- Coverage   88.61%   88.61%   -0.01%     
==========================================
  Files         174      174              
  Lines      127640   127684      +44     
  Branches   127640   127684      +44     
==========================================
+ Hits       113113   113148      +35     
- Misses      12046    12056      +10     
+ Partials     2481     2480       -1     
Flag Coverage Δ
tests 88.61% <79.82%> (-0.01%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Jul 30 '25 12:07 codecov[bot]

✅ Added second reviewer: @valentinewallace

ldk-reviews-bot avatar Jul 31 '25 06:07 ldk-reviews-bot

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review. Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot avatar Aug 02 '25 06:08 ldk-reviews-bot

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review. Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot avatar Aug 04 '25 06:08 ldk-reviews-bot

Basically LGTM!

Could you confirm one thing for me: if we fail to receive an HTLC that's destined for us, it will then be put into the forward_htlcs map keyed with the scid of the previous hop? That's how I'm reading the behavior of fail_htlc_backwards_internal atm. If that's the case, may want to note that on the forward_htlcs field.

Yes, I think this is correct, and the same hold for pending_intercepted_htlcs, AFAICT. I now added respective comments.

tnull avatar Aug 06 '25 08:08 tnull