-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Padding for Payment and Message Blinded Tlvs #3177
base: main
Are you sure you want to change the base?
Conversation
Notes:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3177 +/- ##
==========================================
+ Coverage 88.30% 88.33% +0.02%
==========================================
Files 149 149
Lines 112912 113110 +198
Branches 112912 113110 +198
==========================================
+ Hits 99711 99916 +205
+ Misses 10716 10709 -7
Partials 2485 2485 ☔ View full report in Codecov by Sentry. |
Updated from pr3177.01 to pr3177.02 (diff): Changes:
|
Updated from pr3177.02 to pr3177.03 (diff): Changes:
Diff post rebase: --- a/lightning/src/ln/offers_tests.rs
+++ b/lightning/src/ln/offers_tests.rs
@@ -1789,7 +1789,7 @@ fn test_blinded_path_padding() {
let onion_message = charlie.onion_messenger.next_onion_message_for_peer(david_id).unwrap();
david.onion_messenger.handle_onion_message(&charlie_id, &onion_message);
- let invoice = extract_invoice(david, &onion_message);
+ let (invoice, _) = extract_invoice(david, &onion_message);
assert_eq!(invoice, expected_invoice); |
Updated from pr3177.03 to pr3177.04 (diff):
|
Update: From pr3177.08 to pr3177.09 (diff): Changes based on @jkczyz's feedback:
|
@@ -1772,6 +1772,7 @@ fn packet_payloads_and_keys<T: OnionMessageContents, S: secp256k1::Signing + sec | |||
if let Some(ss) = prev_control_tlvs_ss.take() { | |||
payloads.push((Payload::Forward(ForwardControlTlvs::Unblinded( | |||
ForwardTlvs { | |||
padding: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheBlueMatt One thing that I'm unsure of is whether we only need to pad the blinded path or if we should also pad the unblinded portion of the onion.
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Update: Changes:
Considerations & Shortcomings:
Todo:
|
Update: From pr3177.20 to pr3177.21 (diff): Changes:
|
Update: From pr3177.22 to pr3177.23 (diff): Changes: Fixed the Padding approach.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM, thanks!
lightning/src/blinded_path/utils.rs
Outdated
/// Reads padding to the end, ignoring what's read. | ||
pub(crate) struct Padding {} | ||
/// Represents the padding round off size (in bytes) that is used to pad [`BlindedHop`] | ||
pub const PADDING_ROUND_OFF: usize = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 50? Are common blinded path hops 50 bytes? We should at least write down how big blinded path hops generally are to make sure we're not cutting right in the middle of common sizes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 50? Are common blinded path hops 50 bytes?
Seems this would depend on the received TLVs' size (i.e., how big MessageContext
or PaymentContext
is for the given use case).
We should at least write down how big blinded path hops generally are to make sure we're not cutting right in the middle of common sizes here.
Could you elaborate on this concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is if there's some feature of a blinded path which an observer wants to detect, we don't want to have a constant here that is exactly on the line between that feature and not that feature, otherwise we have gained nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, I was able to determine the common sizes for various message and payment TLVs. You can check out the branch where I introduced the test.
After that, I ran some experiments with different padding sizes. It looks like P=50 and P=70 are both solid options. P=50 gave us 3 unique groups, and P=70 reduced it to 2 while keeping the extra padding fairly minimal.
Here's a table for comparison:
Original Sizes | Rounded (P=50) | Rounded (P=70) |
---|---|---|
35 | 50 | 70 |
70 | 100 | 70 |
10 | 50 | 70 |
28 | 50 | 70 |
62 | 100 | 70 |
96 | 100 | 140 |
96 | 100 | 140 |
96 | 100 | 140 |
27 | 50 | 70 |
42 | 50 | 70 |
26 | 50 | 70 |
26 | 50 | 70 |
128 | 150 | 140 |
53 | 100 | 70 |
Let me know which size you think we should go with! I'm happy to make any adjustments.
Thanks so much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you label the rows by their types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. The new analysis included human_readable_name
, which we should exclude since the length is arbitrary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the updated analysis, the size of the "For Offer" case has changed to 182. Taking this into account, here’s the revised analysis:
The updated findings indicate that P=31 consistently results in the smallest net Blinded Path size for the "For Offer" case, whereas P=29 minimizes the size of the Blinded Path for the "For Refund" case.
Analysis of Sizes and Added Padding for Forward & ReceiveTlvs
Type | Property | Original Sizes | Rounded (P=29) | Rounded (P=30) | Rounded (P=31) | Rounded (P=32) |
---|---|---|---|---|---|---|
Payment ForwardTlvs | ||||||
With no Blinding Override | 29 | 29 x 1 = 29 (No Padding) | 30 x 1 = 30 (+1 size) | 31 x 1 = 31 (+2 size) | 32 x 1 = 32 (+3 size) | |
With Blinding Override | 29 | 29 x 1 = 29 (No Padding) | 30 x 1 = 30 (+1 size) | 31 x 1 = 31 (+2 size) | 32 x 1 = 32 (+3 size) | |
Payment ReceiveTlvs | ||||||
For Offer | 182 | 29 x 7 = 203 (+21 size) | 30 x 7 = 210 (+28 size) | 31 x 6 = 186 (+4 size) | 32 x 6 = 192 (+10 size) | |
For Refund | 110 | 29 x 4 = 116 (+6 size) | 30 x 4 = 120 (+10 size) | 31 x 4 = 124 (+14 size) | 32 x 4 = 128 (+18 size) |
Comparative Analysis of Total Size of Blinded Path for ForwardTlvs (0–10)
For Offer
Number of Forward TLVs | Total Size (P=29) | Total Size (P=30) | Total Size (P=31) | Total Size (P=32) |
---|---|---|---|---|
0 | 203 | 210 | 186 | 192 |
1 | 232 | 240 | 217 | 224 |
2 | 261 | 270 | 248 | 256 |
3 | 290 | 300 | 279 | 288 |
4 | 319 | 330 | 310 | 320 |
5 | 348 | 360 | 341 | 352 |
6 | 377 | 390 | 372 | 384 |
7 | 406 | 420 | 403 | 416 |
8 | 435 | 450 | 434 | 448 |
9 | 464 | 480 | 465 | 480 |
10 | 493 | 510 | 496 | 512 |
For Refund
Number of Forward TLVs | Total Size (P=29) | Total Size (P=30) | Total Size (P=31) | Total Size (P=32) |
---|---|---|---|---|
0 | 116 | 120 | 124 | 128 |
1 | 145 | 150 | 155 | 160 |
2 | 174 | 180 | 186 | 192 |
3 | 203 | 210 | 217 | 224 |
4 | 232 | 240 | 248 | 256 |
5 | 261 | 270 | 279 | 288 |
6 | 290 | 300 | 310 | 320 |
7 | 319 | 330 | 341 | 352 |
8 | 348 | 360 | 372 | 384 |
9 | 377 | 390 | 403 | 416 |
10 | 406 | 420 | 434 | 448 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something looks off when adding the ForwardTlvs
. I'd expect each row to be divisible by P
, but that's not the case for P > 0
. IIUC, each additional ForwardTlvs
should add 2 * P
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
Thanks so much for the nudge!
I’ve updated the table in the comment above, and with the corrected data, it looks like P=31
performs better for the Offer case, while P=29
edges out P=31
for the Refund case.
Curious to hear your thoughts on this!
Btw, I also put together a Python script to analyze the data with a grayscale visualization to better see which values work better across all cases. You can check it out here: Gist Link.
Looking forward to your thought!
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce specific paddings for Offer
& Refund
payee tlvs in pr3177.28
Thanks a lot for all the discussions & pointers!
|
||
/// Represents optional padding for encrypted payloads. | ||
/// Padding is used to ensure payloads have a consistent length. | ||
pub(crate) struct Padding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we split Padding
into a write-side one and a read-side one now that we only use the write-side one here and the read-side one elsewhere in the module? Also because its weird that length
is only used on the write side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! I’m just wondering though, would introducing two separate structs for Padding
add extra complexity to the code?
lightning/src/blinded_path/utils.rs
Outdated
/// Reads padding to the end, ignoring what's read. | ||
pub(crate) struct Padding {} | ||
/// Represents the padding round off size (in bytes) that is used to pad [`BlindedHop`] | ||
pub const PADDING_ROUND_OFF: usize = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 50? Are common blinded path hops 50 bytes?
Seems this would depend on the received TLVs' size (i.e., how big MessageContext
or PaymentContext
is for the given use case).
We should at least write down how big blinded path hops generally are to make sure we're not cutting right in the middle of common sizes here.
Could you elaborate on this concern?
Update: From pr3177.24 to pr3177.25 (diff): Changes:
|
1. This allows setting the length of padding at the time of writing. 2. This will be used in the following commit to allow setting the padding for blinded message paths, and blinded payment paths.
Add a generic `WithPadding` struct to handle padding for `ForwardTlvs` and `ReceiveTlvs` used in `BlindedMessagePath` and `BlindedPaymentPath`. This struct applies padding to the contained TLVs, rounding them off to a specified value. This design provides flexibility in padding TLVs of varying sizes. The `PADDING_ROUND_OFF` value is chosen to be sufficiently large to properly mask the data type of the contained TLVs.
A note of Compact Blinded Paths: Compact Blinded paths are intended to be as short as possible. So to maintain there compactness, we don't apply padding to them.
- This allows using optimal padding for offer & refund case.
Add test to verify blinded message and payment path padding.
Description
This PR introduces padding for
Payment
andMessage
Blinded TLVs to ensure that the size of each packet in the path is uniform.