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 84.50185% with 42 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (b5bfa85) to head (d114291).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	76.71%	29 Missing and 5 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.87%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3868      +/-   ##
==========================================
+ Coverage   89.75%   90.04%   +0.29%     
==========================================
  Files         164      164              
  Lines      132834   136600    +3766     
  Branches   132834   136600    +3766     
==========================================
+ Hits       119229   123006    +3777     
- Misses      10926    10963      +37     
+ Partials     2679     2631      -48

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

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.

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 added 5 commits June 20, 2025 11:53

Rustfmt cleanup

9b6faff

Reduce hold time resolution

5e62df5

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

b0c2e71

Test hold time setting and decoding

5e1b5ef

Update attribution_data tlv type to 1

afc25b0

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 8286744 to afc25b0 Compare June 20, 2025 10:00

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

		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

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!

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!

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

joostjager Jun 20, 2025 •

edited

Loading

TheBlueMatt Jun 20, 2025 •

edited

Loading

tnull left a comment •

edited

Loading