Introduce custom TLV support for OnionMessage #2830
Conversation
WalkthroughThe updates involve the integration of Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/blinded_path/message.rs (4 hunks)
- lightning/src/onion_message/messenger.rs (4 hunks)
- lightning/src/onion_message/packet.rs (2 hunks)
- lightning/src/util/ser_macros.rs (2 hunks)
Additional comments: 13
lightning/src/blinded_path/message.rs (4)
- 25-26: The addition of the
custom_tlvsfield to theForwardTlvsstruct is consistent with the PR's objective to support custom TLVs.- 35-36: The addition of the
custom_tlvsfield to theReceiveTlvsstruct aligns with the PR's goal of extending TLV support.- 67-69: The update to the
blinded_hopsfunction correctly initializescustom_tlvswith an empty vector, which is consistent with the changes made to theForwardTlvsandReceiveTlvsstructs.- 87-87: The changes in the
advance_path_by_onefunction correctly handle thecustom_tlvswhen reading theControlTlvs::Forwardvariant, aligning with the PR's objectives to support custom TLVs.lightning/src/onion_message/packet.rs (2)
- 285-300: The custom TLV decoding logic added to the
ControlTlvsenum is consistent with the PR's objective to support custom TLVs and appears to correctly handle the decoding process.- 312-317: The handling of
custom_tlvsin theControlTlvsenum when constructing the payload format is appropriate and aligns with the changes made to support custom TLVs.lightning/src/onion_message/messenger.rs (5)
- 630-630: The addition of
custom_tlvsto theReceiveControlTlvs::Unblindedvariant aligns with the PR's objective to support custom TLVs.- 635-636: The addition of
custom_tlvsto theForwardControlTlvs::Unblindedvariant is consistent with the changes made toReceiveControlTlvsand supports the PR's goal for custom TLV support.- 1112-1112: The initialization of
custom_tlvswithVec::new()in theForwardTlvsstruct is correct and ensures that the vector is empty by default.- 1123-1123: The addition of
custom_tlvswith an empty vector in theForwardTlvsstruct within theForwardControlTlvs::Unblindedvariant is consistent with the other changes and is correctly implemented.- 1157-1157: The
ReceiveTlvsstruct is correctly updated to includecustom_tlvswith an empty vector, which is in line with the PR's objectives.lightning/src/util/ser_macros.rs (2)
- 475-475: The macro
decode_tlv_stream_with_custom_tlv_decodeis correctly defined and follows the established pattern for TLV stream decoding with the addition of custom TLV handling.- 829-845: The macro
_init_and_read_tlv_stream_with_custom_tlv_decodeis correctly defined and follows the established pattern for initializing and reading a TLV stream with custom TLV decoding.
|
With the current version of PR, I am seeking concept/approach ACKs before I start working on the test suite. A gentle ping, @jkczyz :) |
| @@ -30,6 +32,8 @@ pub(crate) struct ReceiveTlvs { | |||
| /// sending to. This is useful for receivers to check that said blinded path is being used in | |||
| /// the right context. | |||
| pub(crate) path_id: Option<[u8; 32]>, | |||
| /// Custom Tlvs | |||
| pub(crate) custom_tlvs: Vec<(u64, Vec<u8>)>, | |||
There was a problem hiding this comment.
@valentinewallace is this one also LDK-specific in the way payment::ReceiveTlvs is?
In any case, we should really focus on including custom tlvs in payment::ReceiveTlvs (or, not sure if we need "generic custom TLVs" so much as "user provides one Vec<u8> that we put in one TLV).
There was a problem hiding this comment.
@valentinewallace is this one also LDK-specific in the way payment::ReceiveTlvs is?
Yep!
There was a problem hiding this comment.
I was planning to handle the changes related to OnionMessage and BlindedPayment separately.
But it seems more logical now to handle them together.
Introduced custom_tlvs support for payment::ReceiveTlvs in c9d57f8
Let me know if I am on the right track approach-wise!
Thanks!
There was a problem hiding this comment.
@valentinewallace is this one also LDK-specific in the way
payment::ReceiveTlvsis?In any case, we should really focus on including custom tlvs in
payment::ReceiveTlvs(or, not sure if we need "generic custom TLVs" so much as "user provides oneVec<u8>that we put in one TLV).
@TheBlueMatt FYI, the BOLT 12 payment notifications scheme that Jeff wrote up includes blinded payment paths that aren't constructed by the receiver, so this may not be LDK specific anymore.
There was a problem hiding this comment.
We'll have to follow up on that separately, but good call out.
There was a problem hiding this comment.
The second part of the comment here I think still stands, though - there's no reason to support generic custom TLVs, I think, but rather we should focus on supporting reading a single Vec<u8> that users can put whatever they want in (including a TLV stream).
There was a problem hiding this comment.
I'm still not convinced this needs to be a vec of pairs, rather than a single Vec<u8>. Is there a reason you left this as-is?
shaavan
force-pushed
the
#1970/custom-tlvs
branch
from
55098b4
to
c9d57f8
Compare
|
Updated from pr2380.01 to pr2830.02 (diff):
Note: |
|
@shaavan It sounds like you've made significant progress on the PR by addressing previous comments and making adjustments to the |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- lightning/src/blinded_path/message.rs (3 hunks)
- lightning/src/blinded_path/payment.rs (10 hunks)
- lightning/src/ln/blinded_payment_tests.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/msgs.rs (1 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/onion_message/packet.rs (2 hunks)
- lightning/src/util/ser_macros.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- lightning/src/blinded_path/message.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/onion_message/packet.rs
- lightning/src/util/ser_macros.rs
Additional comments: 17
lightning/src/blinded_path/payment.rs (9)
- 19-19: The
Readtrait has been imported fromcrate::io.- 57-58: A
custom_tlvsfield of typeVec<(u64, Vec<u8>)>has been added to theReceiveTlvsstruct to store custom TLV entries.- 127-127: The
Writeabletrait implementation forReceiveTlvshas been updated to include thecustom_tlvsfield with anoptional_vecencoding.- 145-173: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [148-182]
The
Readabletrait implementation forBlindedPaymentTlvshas been modified to decode custom TLVs and populate thecustom_tlvsfield inReceiveTlvs.
- 344-344: The test
compute_payinfohas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 363-363: The test
compute_payinfo_1_hophas been updated similarly to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 417-417: The test
simple_aggregated_htlc_minhas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 468-468: The test
aggregated_htlc_minhas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 523-523: The test
aggregated_htlc_maxhas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.lightning/src/ln/blinded_payment_tests.rs (3)
- 59-59: The addition of
custom_tlvs: Vec::new()initializes an empty vector for custom TLVs. This is consistent with the PR's objective to introduce custom TLV support. However, it's important to ensure that this new field is actually used somewhere in the code or tests, otherwise it's dead code.Verification successful
The
custom_tlvsfield is actively used throughout the codebase, including in tests, message handling, and payment processing. This confirms that the addition of this field is not dead code and aligns with the PR's objectives.* 93-93: The same `custom_tlvs: Vec::new()` addition here. The comment for the `get_blinded_route_parameters` function applies here as well. Ensure that the `custom_tlvs` field is utilized effectively in the codebase. * 136-136: Again, `custom_tlvs: Vec::new()` is added. The same verification is needed to ensure that this field is not just dead code and is actually being used as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if custom_tlvs are used elsewhere in the codebase rg "custom_tlvs" --type rustLength of output: 11880
Verification successful
The
custom_tlvsfield is actively used throughout the codebase, including in payment processing and onion message handling. The initialization ofcustom_tlvswith an empty vector is consistent with its usage in other parts of the code, where it is later populated and utilized. Therefore, the addition ofcustom_tlvs: Vec::new(),in thempp_to_one_hop_blinded_pathfunction aligns with the intended use of custom TLVs within the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Reusing the previous verification script as it applies here too rg "custom_tlvs" --type rustLength of output: 11880
lightning/src/ln/msgs.rs (3)
- 2400-2401: The
custom_tlvsfield in theReceiveTlvsstruct is being matched to_, which indicates that any value is accepted but not used within the block. Please confirm if this is the intended behavior, as it may be necessary to handlecustom_tlvsexplicitly if they are to be used for processing or validation.- 2401-2401: The use of
unwrap_or(0)fortotal_msatassumes that a missingtotal_msatcan be treated as 0. Please ensure that this assumption is valid in all cases, as it could lead to unexpected behavior iftotal_msatis expected to be provided but is not.- 2401-2401: The error handling for
amt_msatusingok_or(DecodeError::InvalidValue)is appropriate and ensures that a missingamt_msatresults in a decode error.lightning/src/ln/channelmanager.rs (2)
- 7942-7942: The addition of the
custom_tlvsfield initialized with an empty vector seems appropriate for optional or extendable features. Ensure that this field is properly populated and utilized in the downstream logic where the blinded payment paths are created.- 7942-7942: Verify if the
custom_tlvsfield should be passed to thecreate_blinded_payment_pathsfunction. If it is intended to be used for handling custom TLVs within the blinded payment paths, it may need to be included in the function call.
|
@shaavan Great to hear that you've addressed the comments from @TheBlueMatt and made the necessary changes to the Action performedReview triggered. |
|
The are still some conceptual kinks to be ironed out before finalising the custom TLV's implementation. |
| }); | ||
| let _padding: Option<utils::Padding> = _padding; | ||
|
|
||
| if let Some(short_channel_id) = scid { | ||
| if payment_secret.is_some() { return Err(DecodeError::InvalidValue) } | ||
| if !custom_tlvs.is_empty() { return Err(DecodeError::InvalidValue) } |
There was a problem hiding this comment.
We dont want to reject unknown odd tlvs.
| @@ -30,6 +32,8 @@ pub(crate) struct ReceiveTlvs { | |||
| /// sending to. This is useful for receivers to check that said blinded path is being used in | |||
| /// the right context. | |||
| pub(crate) path_id: Option<[u8; 32]>, | |||
| /// Custom Tlvs | |||
| pub(crate) custom_tlvs: Vec<(u64, Vec<u8>)>, | |||
There was a problem hiding this comment.
I'm still not convinced this needs to be a vec of pairs, rather than a single Vec<u8>. Is there a reason you left this as-is?
| @@ -472,6 +472,7 @@ macro_rules! decode_tlv_stream { | |||
| /// | |||
| /// [`FixedLengthReader`]: crate::util::ser::FixedLengthReader | |||
| /// [`DecodeError`]: crate::ln::msgs::DecodeError | |||
| #[macro_export] | |||
There was a problem hiding this comment.
Why did you start exporting this?
| @@ -7939,6 +7939,7 @@ where | |||
| max_cltv_expiry, | |||
| htlc_minimum_msat: 1, | |||
| }, | |||
| custom_tlvs: Vec::new(), | |||
There was a problem hiding this comment.
Shouldn't we be exposing this to users so they can start using it :)
Part of #1970
TODO: