Reduce attribution data hold time resolution to 100 ms #3868

joostjager · 2025-06-17T14:56:25Z

Implementing latest iteration of the spec lightning/bolts#1044

ldk-reviews-bot · 2025-06-17T14:56:28Z

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

lightning/src/ln/channel.rs

TheBlueMatt

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

tnull

LGTM, mod one nit and pending comments.

lightning/src/ln/channel.rs

joostjager · 2025-06-18T13:14:59Z

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

codecov · 2025-06-18T13:22:46Z

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3868      +/-   ##
==========================================
+ Coverage   89.75%   90.60%   +0.84%     
==========================================
  Files         164      164              
  Lines      132834   142084    +9250     
  Branches   132834   142084    +9250     
==========================================
+ Hits       119229   128737    +9508     
+ Misses      10926    10660     -266     
- Partials     2679     2687       +8

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

TheBlueMatt · 2025-06-18T14:25:26Z

CI is quite sad

lightning/src/ln/onion_route_tests.rs

TheBlueMatt

There's a few patterns that look worth cleaning up, otherwise LGTM.

TheBlueMatt · 2025-06-19T21:30:07Z

lightning/src/ln/channel.rs

+								now.saturating_sub(timestamp).as_millis()
+									/ HOLD_TIME_GRANULARITY_MS,


Wanna pull the sub into a variable? Or maybe the sub / the granularity?

Done. Renamed to 'unit' / 'units' also.

TheBlueMatt · 2025-06-19T21:32:10Z

lightning/src/ln/onion_route_tests.rs

-		RecipientOnionFields::secret_only(*payment_secret), payment_id).unwrap();
+	nodes[0]
+		.node
+		.send_payment_with_route(


Mind cleaning this up?

Extracted var. I don't think it was necessary.

TheBlueMatt · 2025-06-19T21:32:42Z

lightning/src/ln/onion_route_tests.rs

-			assert!(update_1_0.update_fail_htlcs.len()+update_1_0.update_fail_malformed_htlcs.len()==1 && (update_1_0.update_fail_htlcs.len()==1 || update_1_0.update_fail_malformed_htlcs.len()==1));
+			assert!(
+				update_1_0.update_fail_htlcs.len() + update_1_0.update_fail_malformed_htlcs.len()
+					== 1 && (update_1_0.update_fail_htlcs.len() == 1


bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

Done. Not my preference to fix this in this context though. It was already bad.

lightning/src/ln/onion_route_tests.rs

TheBlueMatt · 2025-06-19T21:50:28Z

lightning/src/ln/onion_route_tests.rs

+				&session_priv,
+			);
+			let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();
+			let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(


If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index b590b123d4..69b63e331c 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -538,16 +538,10 @@ fn test_onion_failure() { &route.paths[0], &session_priv, ); - let recipient_onion_fields = RecipientOnionFields::spontaneous_empty(); - let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads( - &route.paths[0], - 40000, - &recipient_onion_fields, - cur_height, - &None, - None, - None, - ) + let path = &route.paths[0]; + let recipient_fields = RecipientOnionFields::spontaneous_empty(); + let (mut onion_payloads, _htlc_msat, _htlc_cltv) = + build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None) .unwrap(); let mut new_payloads = Vec::new(); for payload in onion_payloads.drain(..) {

Done. Couldn't get them all on one line by extracting vars.

lightning/src/ln/onion_route_tests.rs

TheBlueMatt · 2025-06-20T00:14:24Z

lightning/src/ln/onion_route_tests.rs

+	let amt_to_forward = nodes[1]
+		.node
+		.per_peer_state
+		.read()
+		.unwrap()
+		.get(&nodes[2].node.get_our_node_id())
+		.unwrap()
+		.lock()
+		.unwrap()
+		.channel_by_id
+		.get(&channels[1].2)
+		.unwrap()
+		.context()
+		.get_counterparty_htlc_minimum_msat()
+		- 1;


Lol, can we make this a few variables

Got a 'Tried to violate existing lockorder.'

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() { ); let short_channel_id = channels[1].0.contents.short_channel_id; - let amt_to_forward = nodes[1] - .node - .per_peer_state - .read() - .unwrap() - .get(&nodes[2].node.get_our_node_id()) - .unwrap() - .lock() - .unwrap() - .channel_by_id - .get(&channels[1].2) - .unwrap() - .context() - .get_counterparty_htlc_minimum_msat() - - 1; + let amt_to_forward = { + let (per_peer_state, peer_state); + let node_c_id = nodes[2].node.get_our_node_id(); + let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2); + chan.context().get_counterparty_htlc_minimum_msat() - 1 + }; + let mut bogus_route = route.clone(); let route_len = bogus_route.paths[0].hops.len(); bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

Fixed. Code needed to be slightly different.

TheBlueMatt · 2025-06-20T00:15:44Z

lightning/src/ln/onion_route_tests.rs

-				pubkey: PublicKey::from_slice(&<Vec<u8>>::from_hex("0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c").unwrap()).unwrap(),
+				pubkey: PublicKey::from_slice(
+					&<Vec<u8>>::from_hex(
+						"0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c",


Worth pulling the hex out here and in the next few diff hunks?

Pulled out some constants. Don't think I like it very much.

joostjager · 2025-06-20T06:21:32Z

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

tnull

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

lightning/src/ln/channel.rs

lightning/src/ln/outbound_payment.rs

tnull · 2025-06-19T10:48:56Z

lightning/src/ln/onion_utils.rs

@@ -1223,7 +1223,7 @@ where
 							logger,
 							"Htlc hold time at pos {}: {} ms",
 							route_hop_idx,
-							hold_time
+							hold_time * 100


nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

lightning/src/ln/onion_route_tests.rs

tnull · 2025-06-20T08:48:33Z

lightning/src/ln/onion_route_tests.rs

@@ -1926,19 +1836,24 @@ fn test_always_create_tlv_format_onion_payloads() {
 	}
 }

+const BOB_HEX: &str = "0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c";


Who are these people?

The names of the nodes. Open to suggestions for alternative naming.

In the spec process, it was agreed to report hold time in multiples of 100 ms.

Interop passed and the main specification work is done. Eclair, currently the only implementation supporting attribution data, also uses tlv type 1 already.

TheBlueMatt · 2025-06-20T12:34:13Z

lightning/src/ln/onion_route_tests.rs

-			assert!(update_1_0.update_fail_htlcs.len()+update_1_0.update_fail_malformed_htlcs.len()==1 && (update_1_0.update_fail_htlcs.len()==1 || update_1_0.update_fail_malformed_htlcs.len()==1));
+			let fail_len = update_1_0.update_fail_htlcs.len();
+			let malformed_len = update_1_0.update_fail_malformed_htlcs.len();
+			assert!(fail_len + malformed_len == 1 && (fail_len == 1 || malformed_len == 1));


Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

Fix pushed. Advantage of var extraction.

If two unsigned ints sum to one, one of them has to be equal to one.

joostjager requested a review from tnull June 17, 2025 14:57

joostjager commented Jun 17, 2025

View reviewed changes

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

TheBlueMatt reviewed Jun 17, 2025

View reviewed changes

tnull reviewed Jun 18, 2025

View reviewed changes

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

joostjager force-pushed the coarse-hold-times branch from 2bdfb7e to 2c87246 Compare June 18, 2025 12:41

Rustfmt parts of channel, onion_route_tests and outbound_payment

43340d6

joostjager force-pushed the coarse-hold-times branch from 2c87246 to d114291 Compare June 18, 2025 13:13

joostjager force-pushed the coarse-hold-times branch from d114291 to 8768ead Compare June 18, 2025 14:36

joostjager commented Jun 18, 2025

View reviewed changes

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

joostjager commented Jun 18, 2025

View reviewed changes

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

joostjager force-pushed the coarse-hold-times branch from 8768ead to 39b7248 Compare June 18, 2025 19:24

joostjager requested review from tnull and TheBlueMatt June 19, 2025 07:35

TheBlueMatt reviewed Jun 20, 2025

View reviewed changes

joostjager force-pushed the coarse-hold-times branch from 39b7248 to 8286744 Compare June 20, 2025 06:18

tnull reviewed Jun 20, 2025

View reviewed changes

joostjager force-pushed the coarse-hold-times branch from 8286744 to afc25b0 Compare June 20, 2025 10:00

joostjager requested review from tnull and TheBlueMatt June 20, 2025 10:45

joostjager added 5 commits June 20, 2025 13:54

Rustfmt cleanup

1027929

Reduce hold time resolution

cc97418

In the spec process, it was agreed to report hold time in multiples of 100 ms.

Expose hold times in the PaymentPathFailed event during test

d975986

Test hold time setting and decoding

b97cc8b

Update attribution_data tlv type to 1

0082a70

Interop passed and the main specification work is done. Eclair, currently the only implementation supporting attribution data, also uses tlv type 1 already.

joostjager force-pushed the coarse-hold-times branch from afc25b0 to 0082a70 Compare June 20, 2025 11:54

TheBlueMatt previously approved these changes Jun 20, 2025

View reviewed changes

Remove redundant clause in onion_route_tests assertion

1fc1a14

If two unsigned ints sum to one, one of them has to be equal to one.

joostjager dismissed TheBlueMatt’s stale review via 1fc1a14 June 20, 2025 15:46

		now.saturating_sub(timestamp).as_millis()
		/ HOLD_TIME_GRANULARITY_MS,

Reduce attribution data hold time resolution to 100 ms #3868

Are you sure you want to change the base?

Reduce attribution data hold time resolution to 100 ms #3868

Conversation

joostjager commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joostjager commented Jun 18, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

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!

joostjager Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

TheBlueMatt Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

joostjager commented Jun 20, 2025

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jun 17, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 17, 2025 •

edited

Loading

codecov bot commented Jun 18, 2025 •

edited

Loading

joostjager Jun 20, 2025 •

edited

Loading

TheBlueMatt Jun 20, 2025 •

edited

Loading

tnull left a comment •

edited

Loading