Calculate Trampoline onion packet sizes dynamically. #3333
Conversation
arik-so
force-pushed
the
dynamic_trampoline_onion_sizes
branch
from
f780f8e
to
85c7d76
Compare
lightning/src/ln/onion_utils.rs
Outdated
| 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(); |
There was a problem hiding this comment.
Is it harmful for privacy to not use a fixed length?
There was a problem hiding this comment.
Yes. But we can always pad after the fact, right? Or we can make this an option.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would the code be more confusing if we padded after the fact? I'd kinda prefer to review this commit amidst some more context.
There was a problem hiding this comment.
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 ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
arik-so
force-pushed
the
dynamic_trampoline_onion_sizes
branch
2 times, most recently
from
7f89095
to
b4da6fb
Compare
| let mut payload_len = LengthCalculatingWriter(0); | ||
| p.write(&mut payload_len).expect("Failed to calculate length"); |
There was a problem hiding this comment.
| let mut payload_len = LengthCalculatingWriter(0); | |
| p.write(&mut payload_len).expect("Failed to calculate length"); | |
| let mut payload_len = p.serialized_length(); |
lightning/src/ln/onion_utils.rs
Outdated
| payload_len.0.checked_add(32).expect("Excessive payload size") | ||
| }) | ||
| .try_fold(0usize, |a, b| a.checked_add(b)) | ||
| .expect("Excessive onion length"); |
There was a problem hiding this comment.
Why check + expect rather than just adding? I don't think we're at any real risk of a u64 overflowing here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| let mut chacha = ChaCha20::new(&prng_seed, &[0; 8]); | ||
| chacha.process(&vec![0u8; length as usize], &mut packet_data); | ||
| chacha.process(&vec![0u8; packet_length], &mut packet_data); |
There was a problem hiding this comment.
While you're here mind swapping for process_in_place?
arik-so
force-pushed
the
dynamic_trampoline_onion_sizes
branch
from
b4da6fb
to
f12051f
Compare
Given that Trampoline onion sizes will be variable, this obviates the need to pass the onion size a priori.