Use consistent cltv_expiry_delta in ForwardTlvs
#2831
Conversation
When converting from CounterpartyForwardingInfo to PaymentRelay, the cltv_expiry_delta is copied. Then, when forming a blinded payment path, the value is mutated so that esoteric values don't reveal information about the path. However, the value was only used in computing PaymentConstraints and wasn't actually updated in PaymentRelay. Move the logic for modifying the cltv_expiry_delta to the conversion code to avoid this inconsistency.
WalkthroughThe code update involves refining the logic for converting 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lightning/src/blinded_path/payment.rs (1 hunks)
- lightning/src/routing/router.rs (1 hunks)
Additional comments: 8
lightning/src/blinded_path/payment.rs (6)
- 117-117: The implementation of the
Writeabletrait forForwardTlvs,ReceiveTlvs, andBlindedPaymentTlvsRefappears to be correct and follows the expected pattern for encoding TLV streams.- 117-117: The implementation of the
Readabletrait forBlindedPaymentTlvscorrectly decodes the TLV stream and handles optional and required fields appropriately.- 117-117: The
blinded_hopsfunction correctly chains iterators for public keys and TLV references and uses theutils::construct_blinded_hopsfunction to construct the blinded hops.- 117-117: The
amt_to_forward_msatfunction correctly calculates the amount to forward after fees and handles potential underflow with checked arithmetic.- 117-117: The
compute_payinfofunction aggregates fee and CLTV information across a path of nodes and correctly handles potential overflow and underflow with checked arithmetic.- 117-117: The test cases provided in the
testsmodule cover a range of scenarios, including single-hop and multi-hop paths, and check for correct fee and CLTV delta aggregation. They appear to be comprehensive and correctly assert the expected outcomes.lightning/src/routing/router.rs (2)
- 117-120: The refactoring of the
forwarding_infohandling usingmatchandtry_into()is a good improvement for clarity and error handling.- 124-124: The direct assignment of
cltv_expiry_deltafrompayment_relayafter converting it tou32simplifies the code. However, ensure that the type conversion is safe and will not cause overflow issues, given thatcltv_expiry_deltais expected to be a small number in the context of payment channels.
| impl TryFrom<CounterpartyForwardingInfo> for PaymentRelay { | ||
| type Error = (); | ||
|
|
||
| fn try_from(info: CounterpartyForwardingInfo) -> Result<Self, ()> { | ||
| let CounterpartyForwardingInfo { | ||
| fee_base_msat, fee_proportional_millionths, cltv_expiry_delta | ||
| } = info; | ||
| Self { cltv_expiry_delta, fee_proportional_millionths, fee_base_msat } | ||
|
|
||
| // Avoid exposing esoteric CLTV expiry deltas | ||
| let cltv_expiry_delta = match cltv_expiry_delta { | ||
| 0..=40 => 40, | ||
| 41..=80 => 80, | ||
| 81..=144 => 144, | ||
| 145..=216 => 216, | ||
| _ => return Err(()), | ||
| }; | ||
|
|
||
| Ok(Self { cltv_expiry_delta, fee_proportional_millionths, fee_base_msat }) |
There was a problem hiding this comment.
The error type for the TryFrom trait implementation is a unit type (). This provides no information about why the conversion failed. It would be more helpful to define a custom error type that can provide context on failure.
Additionally, the logic for adjusting cltv_expiry_delta is hard-coded with specific ranges and returns an error for any value not within those ranges. This could be problematic if the valid range of cltv_expiry_delta changes in the future or if there are legitimate cases where values outside these ranges should be allowed. Consider whether this logic could be made more flexible or if additional context should be provided for these constraints.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2831 +/- ##
=======================================
Coverage 88.52% 88.53%
=======================================
Files 114 114
Lines 92090 92093 +3
Branches 92090 92093 +3
=======================================
+ Hits 81526 81534 +8
- Misses 8058 8062 +4
+ Partials 2506 2497 -9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
LGTM, thanks.
I don't think we need to wait for a second reviewer here. Bug or not, if we're dealing with (our) Counterparty('s)ForwardingInfo we should always be hiding the cltv expiry delta via round-ups. We should probably also be doing the same for fees, but we can figure that part out later.
When converting from
CounterpartyForwardingInfotoPaymentRelay, thecltv_expiry_deltais copied. Then, when forming a blinded payment path, the value is mutated so that esoteric values don't reveal information about the path. However, the value was only used in computingPaymentConstraintsand wasn't actually updated inPaymentRelay. Move the logic for modifying thecltv_expiry_deltato the conversion code to avoid this inconsistency.