Split out `receive_htlcs` from the forwarding pipeline #3973

tnull · 2025-07-30T12:22:40Z

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.

ldk-reviews-bot · 2025-07-30T12:22:43Z

👋 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.

codecov · 2025-07-30T12:32:37Z

Codecov Report

❌ Patch coverage is 74.33628% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.00%. Comparing base (8fd7cbf) to head (e681129).
⚠️ Report is 54 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	66.00%	15 Missing and 2 partials ⚠️
lightning/src/ln/onion_route_tests.rs	79.48%	8 Missing ⚠️
lightning/src/ln/payment_tests.rs	83.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3973      +/-   ##
==========================================
+ Coverage   88.93%   89.00%   +0.07%     
==========================================
  Files         174      174              
  Lines      123880   124439     +559     
  Branches   123880   124439     +559     
==========================================
+ Hits       110169   110763     +594     
+ Misses      11260    11202      -58     
- Partials     2451     2474      +23

Flag	Coverage Δ
fuzzing	`22.22% <28.00%> (-0.41%)`	⬇️
tests	`88.83% <74.33%> (+0.07%)`	⬆️

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.

lightning/src/ln/channelmanager.rs

ldk-reviews-bot · 2025-07-31T06:01:22Z

✅ Added second reviewer: @valentinewallace

ldk-reviews-bot · 2025-08-02T06:01:33Z

🔔 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 · 2025-08-04T06:02:14Z

🔔 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.

valentinewallace

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.

lightning/src/ln/onion_route_tests.rs

valentinewallace · 2025-08-04T15:41:56Z

lightning/src/ln/channelmanager.rs

+								continue;
+							}
+						},
+						_ => {},


nit: could debug_assert!(false) here if only to indicate we should never hit this

Hmm, I intentionally let all other cases fall through to the the previous behavior. In particular, it seems that trampoline forwards would also be added under SCID 0 but we wouldn't want to push them to receive_htlcs.

Wouldn't Trampoline forwards also be an AddHTLC? So the bottom _ match arm would still be unreachable. It also seems like we should fail here since any failures against SCID 0 should fail anyway (we won't be able to fail cause no such channel exists?)

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-08-04T15:46:46Z

lightning/src/ln/payment_tests.rs

-		panic!("Expected pending receive");
-	};
+	assert_eq!(nodes[3].node.receive_htlcs.lock().unwrap().len(), 1);
+	match nodes[3].node.receive_htlcs.lock().unwrap().get_mut(0).unwrap() {


nit: could've avoided a bit of rewriting here and below by doing match .. get_mut(0).unwrap() in the first commit

Hm yeah. I don't fully recall why I did it this way. Is it fine for you to leave it as-is now?

valentinewallace · 2025-08-04T16:06:36Z

lightning/src/ln/payment_tests.rs

@@ -681,29 +680,25 @@ fn test_reject_mpp_keysend_htlc_mismatching_secret() {
 	let update_add_3 = update_3.update_add_htlcs[0].clone();
 	nodes[3].node.handle_update_add_htlc(node_c_id, &update_add_3);
 	commitment_signed_dance!(nodes[3], nodes[2], update_3.commitment_signed, false, true);
-	expect_htlc_failure_conditions(nodes[3].node.get_and_clear_pending_events(), &[]);


I'm not clear on the reason for removing this, mind explaining? I thought there shouldn't be a behavior change here.

Sure! The event queue is actually empty here, so this is a no-op, which we simply made a bit more explicit with the assert below.

Happy to drop the diff, but FWIW it might be worth cleaning up all instances of expect_htlc_failure_conditions(.., &[]); ?

Probably fine, in the future I think it would be a bit more reviewable to either separate out or note in the commit message.

Previously, some of the tests manipulating `forward_htlcs` were just iterating, but not actually checking whether the expected entries were present and they were actually changed. Here we make these test cases more strict.

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

tnull · 2025-08-06T08:48:31Z

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.

valentinewallace · 2025-08-06T16:53:26Z

lightning/src/ln/onion_route_tests.rs

-				}) => forward_info.outgoing_cltv_value += 1,
-				_ => {},
-			}
+	nodes[1].node.process_pending_update_add_htlcs();


I think this line gets removed in a later commit? It doesn't seem to belong in this commit.

valentinewallace · 2025-08-06T16:56:55Z

lightning/src/ln/channelmanager.rs

@@ -2537,7 +2539,7 @@ pub struct ChannelManager<
 	/// See `ChannelManager` struct-level documentation for lock order requirements.
 	pending_outbound_payments: OutboundPayments,

-	/// SCID/SCID Alias -> forward infos. Key of 0 means payments received.


Could document that 0 actually means trampoline now.

valentinewallace · 2025-08-06T17:13:55Z

lightning/src/ln/channelmanager.rs

@@ -15091,6 +15124,8 @@ where
 			}
 		}

+		let receive_htlcs = self.receive_htlcs.lock().unwrap();


Doesn't it break downgrades to only write the new vec, and not also write the receives in the legacy forward_htlcs?

TheBlueMatt · 2025-08-06T17:40:58Z

lightning/src/ln/onion_route_tests.rs

+			assert_eq!(nodes[1].node.forward_htlcs.lock().unwrap().len(), 1);
+			if let Some((_, pending_forwards)) =
+				nodes[1].node.forward_htlcs.lock().unwrap().iter_mut().next()
+			{
+				assert_eq!(pending_forwards.len(), 1);
+				match pending_forwards.get_mut(0).unwrap() {


This repeated pattern really looks like it could be in a test util function or macro.

TheBlueMatt · 2025-08-06T19:51:07Z

lightning/src/ln/channelmanager.rs

+								continue;
+							}
+						},
+						_ => {},


Wouldn't Trampoline forwards also be an AddHTLC? So the bottom _ match arm would still be unreachable. It also seems like we should fail here since any failures against SCID 0 should fail anyway (we won't be able to fail cause no such channel exists?)

TheBlueMatt · 2025-08-06T19:53:48Z

lightning/src/ln/channelmanager.rs

+	#[cfg(test)]
+	pub(super) receive_htlcs: Mutex<Vec<HTLCForwardInfo>>,
+	#[cfg(not(test))]
+	receive_htlcs: Mutex<Vec<HTLCForwardInfo>>,


ISTM we should change the type here because we still have the issues from the combined map (panics in process_forward_htlcs for receives and panics in process_receive_htlcs for forwards) but now dont have the reason for the (one map). Also the panic messages in those methods are wrong now.

tnull added this to Weekly Goals Jul 30, 2025

tnull self-assigned this Jul 30, 2025

tnull added the weekly goal Someone wants to land this this week label Jul 30, 2025

tnull moved this to Goal: Merge in Weekly Goals Jul 30, 2025

ldk-reviews-bot requested a review from valentinewallace July 30, 2025 12:32

martinsaposnic reviewed Jul 30, 2025

View reviewed changes

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

tnull force-pushed the 2025-07-split-out-receive-htlcs branch from b360839 to 2f5e940 Compare July 30, 2025 15:00

martinsaposnic reviewed Jul 30, 2025

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

tnull removed the request for review from valentinewallace July 31, 2025 06:01

ldk-reviews-bot requested a review from valentinewallace July 31, 2025 06:01

valentinewallace reviewed Aug 4, 2025

View reviewed changes

tnull added 5 commits August 5, 2025 10:48

Make tests touching forward_htlcs more strict

5272201

Previously, some of the tests manipulating `forward_htlcs` were just iterating, but not actually checking whether the expected entries were present and they were actually changed. Here we make these test cases more strict.

f One more length assertion

28dcf35

Move receiving HTLCs to a receive_htlcs field

35d1098

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

f Persist after handling receive_htlcs

e6a00fc

f Debug-assert that not both receive htlc Vecs are set

c6ba5ff

tnull force-pushed the 2025-07-split-out-receive-htlcs branch from cad3d32 to c6ba5ff Compare August 5, 2025 08:51

f Add comments noting that failures will be added to forward_htlcs

e681129

tnull requested review from valentinewallace and TheBlueMatt August 6, 2025 08:48

valentinewallace reviewed Aug 6, 2025

View reviewed changes

TheBlueMatt reviewed Aug 6, 2025

View reviewed changes

Split out receive_htlcs from the forwarding pipeline #3973

Are you sure you want to change the base?

Split out receive_htlcs from the forwarding pipeline #3973

Conversation

tnull commented Jul 30, 2025

Uh oh!

ldk-reviews-bot commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 31, 2025

Uh oh!

ldk-reviews-bot commented Aug 2, 2025

Uh oh!

ldk-reviews-bot commented Aug 4, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Split out `receive_htlcs` from the forwarding pipeline #3973

Split out `receive_htlcs` from the forwarding pipeline #3973

ldk-reviews-bot commented Jul 30, 2025 •

edited

Loading

codecov bot commented Jul 30, 2025 •

edited

Loading