Calculate Trampoline onion packet sizes dynamically. #3333

arik-so · 2024-09-24T02:10:22Z

Given that Trampoline onion sizes will be variable, this obviates the need to pass the onion size a priori.

Builds on top of #3446.

valentinewallace · 2024-09-24T02:14:17Z

lightning/src/ln/onion_utils.rs

+	let packet_length: usize = payloads
+		.iter()
+		.map(|p| {
+			let mut payload_len = LengthCalculatingWriter(0);
+			p.write(&mut payload_len).expect("Failed to calculate length");
+			payload_len.0 + 32
+		})
+		.sum();


Is it harmful for privacy to not use a fixed length?

Yes. But we can always pad after the fact, right? Or we can make this an option.

Given that the recipient has to read the entire onion anyway, and therefore its potential size is limited by how many outer onion hops came before, and that there are not usually many Trampoline hops, there is a potential limitation of privacy even so.

Would the code be more confusing if we padded after the fact? I'd kinda prefer to review this commit amidst some more context.

Hm, I do think padding after the fact would be more complicated. The context is mostly integration tests against ACINQ, where the current version does not appear to be padding the onion to a fixed size, which we should actually bring up to Bastien. It didn't occur to me yesterday.

codecov · 2024-09-24T02:19:43Z

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 89.60%. Comparing base (66fb520) to head (b4da6fb).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/onion_utils.rs	0.00%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3333      +/-   ##
==========================================
- Coverage   89.66%   89.60%   -0.06%     
==========================================
  Files         126      126              
  Lines      102676   102766      +90     
  Branches   102676   102766      +90     
==========================================
+ Hits        92062    92088      +26     
- Misses       7894     7943      +49     
- Partials     2720     2735      +15

Flag	Coverage Δ
	`89.60% <0.00%> (-0.03%)`	⬇️

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.

TheBlueMatt

Feel free to squash when you next push.

lightning/src/ln/onion_utils.rs

TheBlueMatt · 2024-10-08T20:35:18Z

lightning/src/ln/onion_utils.rs

+			payload_len.0.checked_add(32).expect("Excessive payload size")
+		})
+		.try_fold(0usize, |a, b| a.checked_add(b))
+		.expect("Excessive onion length");


Why check + expect rather than just adding? I don't think we're at any real risk of a u64 overflowing here.

frankly, I've been thinking the same thing. Currently it's in a bit of a limbo state, because even with the checked addition, it's still expecting. So either I should surface it to an Error struct, or allow it to throw immediately. Though you're also right, it's unlikely to happen outside of a fuzz scenario.

I'm not sure its possible anyway? We'd have to have a huge custom TLV and we should reject that long before we get to this point (or just blame the user for trying to stuff 4GB of garbage down an onion).

lightning/src/ln/onion_utils.rs

tankyleo · 2024-12-18T20:10:54Z

lightning/src/ln/onion_utils.rs

+		"Trampoline onion packet must be smaller than outer onion"
+	);
+
+	let packet_length = length.unwrap_or(minimum_packet_length as u16) as usize;


Suggested change

let packet_length = length.unwrap_or(minimum_packet_length as u16) as usize;

let packet_length = length.map(|l| usize::from(l)).unwrap_or(minimum_packet_length);

usize::from guarantees no information loss, as usize does not.

tankyleo · 2024-12-18T20:23:47Z

lightning/src/ln/onion_utils.rs

 ) -> Result<msgs::TrampolineOnionPacket, ()> {
-	let mut packet_data = vec![0u8; length as usize];
+	let minimum_packet_length: usize = payloads.iter().map(|p| p.serialized_length() + 32).sum();


Nit: doesn't seem like : usize is needed, but I let you make the decision on style - from my point of view, we are talking about lengths here, so not surprising that the type is usize.

TheBlueMatt · 2025-01-23T21:19:02Z

lightning/src/ln/onion_utils.rs


+	assert!(


Please no hard assertions unless its gonna result in us losing money. debug assert and handle the err.

valentinewallace · 2025-01-28T16:11:41Z

LGTM after squash!

For outbound Trampoline payments, we will need to generate onion packets whose size matches their content. This allows them to be included in individual hops at arbitrary positions. A future spec change may involve defining a static size to improve privacy from Trampoline nodes.

valentinewallace

Thought I ACK'd already but guess not

arik-so requested a review from valentinewallace September 24, 2024 02:10

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from f780f8e to 85c7d76 Compare September 24, 2024 02:12

valentinewallace reviewed Sep 24, 2024

View reviewed changes

arik-so force-pushed the dynamic_trampoline_onion_sizes branch 2 times, most recently from 7f89095 to b4da6fb Compare September 25, 2024 01:53

TheBlueMatt reviewed Oct 8, 2024

View reviewed changes

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

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from b4da6fb to f12051f Compare October 27, 2024 07:50

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from f12051f to bcfab4b Compare November 21, 2024 17:11

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from bcfab4b to 2056f92 Compare December 5, 2024 01:19

arik-so mentioned this pull request Dec 5, 2024

Trampoline #2299

Open

31 tasks

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from 2056f92 to bc899ce Compare December 5, 2024 15:54

arik-so mentioned this pull request Dec 5, 2024

Trampoline Payload Construction Method #3386

Merged

arik-so force-pushed the dynamic_trampoline_onion_sizes branch 3 times, most recently from 03ce3c0 to 4077d64 Compare December 12, 2024 17:15

tankyleo reviewed Dec 18, 2024

View reviewed changes

TheBlueMatt added the weekly goal Someone wants to land this this week label Jan 23, 2025

TheBlueMatt reviewed Jan 23, 2025

View reviewed changes

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from 4077d64 to b679944 Compare January 28, 2025 05:41

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from b679944 to 06aea45 Compare January 28, 2025 16:38

arik-so force-pushed the dynamic_trampoline_onion_sizes branch from 06aea45 to 7c6f220 Compare January 28, 2025 19:00

TheBlueMatt approved these changes Jan 28, 2025

View reviewed changes

valentinewallace approved these changes Jan 28, 2025

View reviewed changes

valentinewallace merged commit b1fc7d8 into lightningdevkit:main Jan 28, 2025
24 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate Trampoline onion packet sizes dynamically. #3333

Calculate Trampoline onion packet sizes dynamically. #3333

arik-so commented Sep 24, 2024 •

edited

Loading

valentinewallace Sep 24, 2024

arik-so Sep 24, 2024

arik-so Sep 24, 2024

valentinewallace Sep 24, 2024

arik-so Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Oct 8, 2024

arik-so Oct 10, 2024

TheBlueMatt Oct 17, 2024

tankyleo Dec 18, 2024

tankyleo Dec 18, 2024

TheBlueMatt Jan 23, 2025

valentinewallace commented Jan 28, 2025

valentinewallace left a comment

	let packet_length = length.unwrap_or(minimum_packet_length as u16) as usize;
	let packet_length = length.map(\|l\| usize::from(l)).unwrap_or(minimum_packet_length);

Calculate Trampoline onion packet sizes dynamically. #3333

Calculate Trampoline onion packet sizes dynamically. #3333

Conversation

arik-so commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Jan 28, 2025

valentinewallace left a comment

Choose a reason for hiding this comment

arik-so commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading