Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

shaavan
Copy link
Member

@shaavan shaavan commented Jul 13, 2024

Description

This PR introduces padding for Payment and Message Blinded TLVs to ensure that the size of each packet in the path is uniform.

@shaavan
Copy link
Member Author

shaavan commented Jul 13, 2024

Notes:

  1. Padding Implementation:
    • The padding field is now expanded to contain Option<Vec>. This change allows the inclusion of padding within the packet while retaining the flexibility to leave the padding as None.
  2. Impact on Path Length:
    • With the added padding, the length of each individual packet increases, which results in a reduction of the maximum path length. Consequently, the max_path_length in forward_check_failures has been adjusted from 18 to 17.

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 98.53659% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.33%. Comparing base (85d1e5f) to head (610dd41).

Files with missing lines Patch % Lines
lightning/src/blinded_path/utils.rs 91.42% 1 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@jkczyz jkczyz self-requested a review July 15, 2024 20:03
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Jul 18, 2024

Updated from pr3177.01 to pr3177.02 (diff):
Addressed @jkczyz's comments.

Changes:

  1. Reordered commits and separated the update to the Padding struct into a separate commit.
  2. Updated the Padding struct to contain a usize instead of a Vec to save on heap allocation, and made the Writable implementation more efficient.
  3. Introduced two f: commits to use iter clone instead of Vec allocation, keeping the code efficient.
  4. Introduced a test and a test utility to verify that blinded message and payment paths are properly padded, ensuring a consistent payload size.

@shaavan
Copy link
Member Author

shaavan commented Jul 18, 2024

Updated from pr3177.02 to pr3177.03 (diff):

Changes:

  1. Rebase on main to fix ci.

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

lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Jul 22, 2024

Updated from pr3177.03 to pr3177.04 (diff):
Addressed @jkczyz comments

  1. Refactor max_length calculation to be cleaner.
  2. Introduce wrapper struct instead of tuple to make the code clearer to understand.
  3. Move the padding testing to onion_message/functional_tests.rs to simplify the setup, and test the code at a more appropriate place.

lightning/src/onion_message/packet.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/packet.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/packet.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/functional_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Jul 23, 2024

Updated from pr3177.04 to pr3177.05 (diff):

Updates:

  1. Rebase on main.

@shaavan
Copy link
Member Author

shaavan commented Jul 23, 2024

Updated from pr3177.05 to pr3177.06 (diff):
Addressed @jkczyz comments

Changes:

  1. Squash commits.
  2. Introduce a generic struct WithPadding to keep the code DRY.
  3. Introduce a test for Blinded Payment Paths.
  4. Use saturating sub in padding length calculation to avoid overflows.

lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/blinded_payment_tests.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Jul 26, 2024

Updated from pr3177.06 to pr3177.07 (diff):
Addressed @jkczyz comments

  1. Update comments.
  2. Introduce debug_assert!() in Writeable for WithPadding.
  3. Properly assert!() the condition in test.

@shaavan
Copy link
Member Author

shaavan commented Jul 27, 2024

Updated from pr3177.07 to pr3177.08 (diff):
Addressed @jkczyz comments

Changes:

  1. Introduce an f: commit with an alternative approach to WithPadding.
  2. The new approach introduces padding as an optional field within the ForwardTlvs and the ReceiveTlvs struct.

lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Aug 1, 2024

Update: From pr3177.08 to pr3177.09 (diff):

Changes based on @jkczyz's feedback:

  1. Fixed the imports.
  2. Removed BlindedPaymentTlvsRef and replaced it with BlindedPaymentTlvs. The former contained an immutable reference to the underlying TLVs, but the new implementation required mutability. Therefore, I transitioned to using BlindedPaymentTlvs directly, making BlindedPaymentTlvsRef redundant.

lightning/src/onion_message/packet.rs Outdated Show resolved Hide resolved
@@ -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,
Copy link
Contributor

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.

lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Aug 14, 2024

Update: From pr3177.09 to pr3177.10 (diff):

Changes based on @jkczyz's feedback:

  1. Rename pad_tlvs -> pad_to_length.

@shaavan
Copy link
Member Author

shaavan commented Sep 27, 2024

Update: From pr3177.18 to pr3177.19 (diff):

Changes:

  1. Rebase on main to resolve merge conflicts.

@github-advanced-security
Copy link

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.

@shaavan
Copy link
Member Author

shaavan commented Oct 1, 2024

Update:
From pr3177.19 to pr3177.20 (diff):
Addressed feedback from @TheBlueMatt and @jkczyz.

Changes:

  1. Simplified the padding approach by removing padding as a field from {Forward, Receive} TLVs.
  2. Introduced a WithPadding generic that handles padding for the given TLVs.

Considerations & Shortcomings:

  • Instead of padding to a maximum value, WithPadding pads the TLVs to a rounded value. This avoids wasting space for very small packets while providing flexibility for future TLV expansions.
  • The current padding round-off value is 50 bytes. I believe this should offer enough buffer to obscure the internal TLV types, but would love to get feedback on a more optimal value.
  • This approach doesn't safeguard against the improper use of a 1 TLV record within the TLVs themselves. We may need to add a comment to prevent this misuse.

Todo:

  • Currently, this approach also pads the compact blinded path. I'll address this in the next update.

@shaavan
Copy link
Member Author

shaavan commented Oct 2, 2024

Update: From pr3177.20 to pr3177.21 (diff):
Addressed @TheBlueMatt comment

Changes:

  1. Update code so that we would not be padding the compact blinded paths.
  2. Introduce a test to check this condition.

@shaavan
Copy link
Member Author

shaavan commented Oct 5, 2024

Update: From pr3177.21 to pr3177.22 (diff):

Changes:

  1. Rebase on main to resolve merge conflicts.

@shaavan
Copy link
Member Author

shaavan commented Oct 5, 2024

Update: From pr3177.22 to pr3177.23 (diff):

Changes:

Fixed the Padding approach.

  1. Previous version: Packets were padded individually based on compact forward TLVs. This incorrectly led to padding ReceiveTlvs, even when they belonged to a compact blinded path.
  2. Fix: Now, the entire blinded path remains unpadded if any short_channel_id is present. This ensures correct handling of compact paths and eliminates unnecessary padding.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM, thanks!

/// 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;
Copy link
Collaborator

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.

Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Member Author

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!

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

@shaavan shaavan Dec 18, 2024

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

Copy link
Contributor

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.

Copy link
Member Author

@shaavan shaavan Dec 23, 2024

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!

Copy link
Member Author

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 {
Copy link
Collaborator

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.

Copy link
Member Author

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 Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
/// 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;
Copy link
Contributor

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?

lightning/src/blinded_path/utils.rs Outdated Show resolved Hide resolved
@shaavan
Copy link
Member Author

shaavan commented Oct 10, 2024

Update: From pr3177.23 to pr3177.24 (diff):

Addressed @jkczyz comments

Changes:

  1. Fix typo, and visibility.

@shaavan
Copy link
Member Author

shaavan commented Oct 30, 2024

Update: From pr3177.24 to pr3177.25 (diff):
Addressed comments from @jkczyz and @TheBlueMatt.

Changes:

  1. Introduced different padding round-off for message and payment TLVs.
  2. Padding sizes were chosen based on the discussion in this thread.

@shaavan
Copy link
Member Author

shaavan commented Nov 25, 2024

Update: From pr3177.25 to pr3177.26 (diff):
Addressed @jkczyz comments

Changes:

  1. Update the Padding for Payment Tlvs to 35, according to this discussion.

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.
@shaavan
Copy link
Member Author

shaavan commented Dec 25, 2024

Update: From pr3177.26 to pr3177.27 (diff):

Changes:

  1. Rebase on main.

- This allows using optimal padding for offer & refund case.
Add test to verify blinded message and payment path padding.
@shaavan
Copy link
Member Author

shaavan commented Dec 25, 2024

Update: From pr3177.27 to pr3177.28 (diff):
Addressed @jkczyz comments

Changes:

  1. Introduce specific paddings for payee_tlvs based on if they belong to a Refund or an Offer.
  2. Introduce test to check for the padding in case of Offer & Refund.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants