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

Merged
merged 7 commits into from
Mar 6, 2025

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

Project coverage is 88.53%. Comparing base (1434e9c) to head (758d06d).

Files with missing lines Patch % Lines
lightning/src/blinded_path/utils.rs 94.44% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3177      +/-   ##
==========================================
- Coverage   88.54%   88.53%   -0.01%     
==========================================
  Files         149      149              
  Lines      114579   114704     +125     
  Branches   114579   114704     +125     
==========================================
+ Hits       101457   101558     +101     
- Misses      10633    10650      +17     
- Partials     2489     2496       +7     

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

@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.

@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.

@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.

@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.

@@ -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.

@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 Jan 20, 2025

Update: From pr3177.28 to pr3177.29 (diff):
Addressed @jkczyz, @TheBlueMatt comments

Changes:

  1. Reverted individual padding for offer, and refund payment tlvs, and updated the value 35 -> 30.
  2. Update padding logic so that we also account for TLV Overhead introduced by padding TLV type and encryption data.
  3. Remove Padding Readable, as the value is never read after use.
  4. Update struct name, Padding -> BlindedPathPadding & documentation to be specific, and clarifying the TLV type 1 usage.

@shaavan
Copy link
Member Author

shaavan commented Jan 31, 2025

Update: From pr3177.29 to pr3177.30 (diff):
Addressed @jkczyz comments

Changes:

  1. Updated the comment on the reasoning behind not reading padding.
  2. Update TLV_OVERHEAD to only account for padding type and length bytes.
  3. Reverted changes to the is_padded function.

@shaavan
Copy link
Member Author

shaavan commented Jan 31, 2025

Update: From pr3177.30 to pr3177.31 (diff):

Changes:

  1. Rebase on main to resolve to merge conflicts.

@shaavan
Copy link
Member Author

shaavan commented Mar 4, 2025

Update: From pr3177.31 to pr3177.32 (diff):

Changes:

  1. Squash commits, and expands commit message to make their purpose clearer.

@shaavan
Copy link
Member Author

shaavan commented Mar 4, 2025

Update: From pr3177.32 to pr3177.33 (diff):

Changes:

  1. Rebase on main, to resolve merge conflicts.

TheBlueMatt
TheBlueMatt previously approved these changes Mar 5, 2025
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.

LGTM, @jkczyz you wanna take another look?

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Looks like rustfmt CI checks are failing. Otherwise, just one nit and LGTM.

@shaavan
Copy link
Member Author

shaavan commented Mar 5, 2025

Update: From pr3177.33 to pr3177.34 (diff):
Addressed @jkczyz comments:

  1. Fix rust formatting
  2. Addressed nit suggestion

shaavan added 7 commits March 6, 2025 02:00
Renaming the struct to `BlindedPathPadding` makes its purpose explicit,
clarifying that it is specifically used for `Blinded{Message/Payment}Paths`.
Padding serves as filler data with no explicit use case, making it
more efficient to ignore it rather than read and discard it.
This change simplifies the code by removing unnecessary `Readable`
implementation for padding.
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 `BlindedPathWithPadding` 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.
Add test to verify blinded message and payment path padding.
@shaavan
Copy link
Member Author

shaavan commented Mar 5, 2025

Update: From pr3177.34 to pr3177.35 (diff):

Changes:

  1. Rebase on main, resolving merge conflicts.

@TheBlueMatt TheBlueMatt merged commit 43de15e into lightningdevkit:main Mar 6, 2025
25 of 27 checks passed
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