Stop sending `channel_update` in onion failures #3345

tankyleo · 2024-10-01T03:53:06Z

codecov · 2024-10-01T03:59:43Z

Codecov Report

Attention: Patch coverage is 86.88525% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.55%. Comparing base (c7627df) to head (9e30f25).
Report is 267 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	82.97%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3345      +/-   ##
==========================================
- Coverage   89.67%   89.55%   -0.12%     
==========================================
  Files         126      127       +1     
  Lines      103165   103469     +304     
  Branches   103165   103469     +304     
==========================================
+ Hits        92510    92660     +150     
- Misses       7935     8104     +169     
+ Partials     2720     2705      -15

Flag	Coverage Δ
	`?`

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.

dunxen

Thanks! Just skimmed the diff for now and will give it a proper look later.

Don't worry about the failing mutants CI job as the diff is triggering a bunch of existing missed mutants.

lightning/src/ln/channelmanager.rs

vincenzopalazzo

I was reviewing this code during the Tokyo summit last week. Since you've already done the work, I added a couple of comments. Some might be unnecessary, and others are just to help me understand your changes 😄

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

vincenzopalazzo · 2024-10-02T15:38:28Z

lightning/src/ln/channelmanager.rs

+										let failure_code = 0x1000|7;
+										let data = self.get_htlc_inbound_temp_fail_data(failure_code);


IMHO there was no reason to change the function return type, is it?

In case this function failed to fetch the channel_update, it previously indicated this to the caller by returning an 0x4000 error, and the caller would reassign the 0x1000 code with the 0x4000 code.

In case the channel_update fetch was successful, the returned error code was the same as the one passed in.

This function no longer has this potential fetch failure, so it will never reassign the error code value to a different value. So we don't need to return an error code value any longer.

Yeah I noted it, and make sense of your explaination. Probably just bias because the previous function was more "elegant" from the code point of view. Thanks for explaination

vincenzopalazzo · 2024-10-02T15:38:50Z

lightning/src/ln/channelmanager.rs

+							let failure_code = 0x1000|7;
+							let data = self.get_htlc_inbound_temp_fail_data(failure_code);


lightning/src/ln/onion_route_tests.rs

tankyleo · 2024-10-02T21:22:59Z

I was reviewing this code during the Tokyo summit last week. Since you've already done the work, I added a couple of comments. Some might be unnecessary, and others are just to help me understand your changes 😄

Sorry I should have understood better, thank you for the review !

dunxen · 2024-10-04T08:04:03Z

lightning/src/ln/channelmanager.rs

+			// See https://github.com/lightning/bolts/blob/341ec84/04-onion-routing.md?plain=1#L1008
+			0u16.write(&mut enc).expect("Writes cannot fail");
+		}
+		(0u16).write(&mut enc).expect("Writes cannot fail");


I know it seems kind of obvious for no channel update, but could we perhaps link to https://github.com/lightning/bolts/blob/247e83d528a2a380e533e89f31918d7b0ce6a0c1/04-onion-routing.md?plain=1#L1414-L1415 in a comment?

Sorry for only seeing this now!

No problem you are right, I think it's worth adding this reference.

Please see below, I added the link in the two places where it is relevant.

I only included the first 7 characters of the commit hash, partly to keep it consistent with the link further above in the location you pointed out.

Per <lightning/bolts#1173>. Fixes lightningdevkit#3277

TheBlueMatt

Damn, I was hoping this would get us some nice lock cleanups, but oh well, its still fixing some bugs in phantom and removing code.

TheBlueMatt · 2024-10-11T20:28:50Z

lightning/src/ln/onion_route_tests.rs

 	let mut fail_conditions = PaymentFailedConditions::new()
 		.blamed_scid(phantom_scid)
-		.expected_htlc_error_data(0x2000 | 2, &[]);
+		.expected_htlc_error_data(0x1000 | 13, &err_data);


valentinewallace

Nice!!

dunxen reviewed Oct 1, 2024

View reviewed changes

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

tankyleo commented Oct 2, 2024

View reviewed changes

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

vincenzopalazzo reviewed Oct 2, 2024

View reviewed changes

tankyleo force-pushed the onion-channel-updates branch from 1d29256 to 66f1825 Compare October 4, 2024 04:54

dunxen reviewed Oct 4, 2024

View reviewed changes

Stop sending channel_update in onion failures

9e30f25

Per <lightning/bolts#1173>. Fixes lightningdevkit#3277

tankyleo force-pushed the onion-channel-updates branch from 66f1825 to 9e30f25 Compare October 5, 2024 03:42

TheBlueMatt approved these changes Oct 11, 2024

View reviewed changes

TheBlueMatt linked an issue Oct 11, 2024 that may be closed by this pull request

Replace temporary_channel_failure with temporary_node_failure for inbound-channel failures #1369

Closed

valentinewallace approved these changes Nov 26, 2024

View reviewed changes

valentinewallace merged commit 70ace81 into lightningdevkit:main Nov 26, 2024
18 of 20 checks passed

tankyleo deleted the onion-channel-updates branch December 13, 2024 02:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop sending `channel_update` in onion failures #3345

Stop sending `channel_update` in onion failures #3345

tankyleo commented Oct 1, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

dunxen left a comment

vincenzopalazzo left a comment •

edited

Loading

vincenzopalazzo Oct 2, 2024

tankyleo Oct 2, 2024 •

edited

Loading

vincenzopalazzo Oct 3, 2024

vincenzopalazzo Oct 2, 2024

tankyleo commented Oct 2, 2024

dunxen Oct 4, 2024

tankyleo Oct 4, 2024

tankyleo Oct 5, 2024

TheBlueMatt left a comment

TheBlueMatt Oct 11, 2024

valentinewallace left a comment

		let failure_code = 0x1000\|7;
		let data = self.get_htlc_inbound_temp_fail_data(failure_code);

Stop sending channel_update in onion failures #3345

Stop sending channel_update in onion failures #3345

Conversation

tankyleo commented Oct 1, 2024

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment • edited Loading

Choose a reason for hiding this comment

vincenzopalazzo Oct 2, 2024

Choose a reason for hiding this comment

tankyleo Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

vincenzopalazzo Oct 3, 2024

Choose a reason for hiding this comment

vincenzopalazzo Oct 2, 2024

Choose a reason for hiding this comment

tankyleo commented Oct 2, 2024

dunxen Oct 4, 2024

Choose a reason for hiding this comment

tankyleo Oct 4, 2024

Choose a reason for hiding this comment

tankyleo Oct 5, 2024

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Oct 11, 2024

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Stop sending `channel_update` in onion failures #3345

Stop sending `channel_update` in onion failures #3345

codecov bot commented Oct 1, 2024 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading

tankyleo Oct 2, 2024 •

edited

Loading