Route blinding: add min_final_cltv_delta to aggregated CLTV delta
#2856
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates focus on enhancing the 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/blinded_path/mod.rs (2 hunks)
- lightning/src/blinded_path/payment.rs (7 hunks)
- lightning/src/ln/blinded_payment_tests.rs (4 hunks)
- lightning/src/routing/router.rs (3 hunks)
Additional comments: 15
lightning/src/blinded_path/mod.rs (3)
- 88-96: The addition of
min_final_cltv_expiry_deltain theone_hop_for_paymentmethod correctly implements the requirement to include the minimum final CLTV expiry delta in the calculation for blinded payments. This change ensures that the CLTV delta calculation for route blinding is accurate.- 111-119: The
new_for_paymentmethod has been updated to include themin_final_cltv_expiry_deltaparameter, which is essential for correctly calculating the CLTV expiry delta for blinded payments. This modification aligns with the PR's objective to correct the oversight in the handling ofmin_final_cltv_delta.- 117-119: The
compute_payinfofunction call within thenew_for_paymentmethod correctly includes themin_final_cltv_expiry_deltaparameter. This ensures that the calculation for the CLTV expiry delta in blinded payments takes into account the minimum final CLTV expiry delta, aligning with the PR's objectives.lightning/src/blinded_path/payment.rs (6)
- 218-219: The modification to the
compute_payinfofunction to include themin_final_cltv_expiry_deltaparameter is correct. This change is necessary for accurately initializing thecltv_expiry_deltavariable within the function, ensuring the inclusion of the minimum final CLTV expiry delta in the aggregated CLTV delta calculation.- 344-347: The test case
compute_payinfoinpayment.rshas been correctly updated to include themin_final_cltv_expiry_deltaparameter when calling thecompute_payinfofunction. This ensures that the tests validate the correct handling of themin_final_cltv_delta, aligning with the PR's objectives.- 361-364: The test case
compute_payinfo_1_hophas been correctly updated to pass themin_final_cltv_expiry_deltaparameter to thecompute_payinfofunction. This update is necessary for testing the function's behavior with the inclusion of the minimum final CLTV expiry delta in the calculation.- 415-415: The test case
simple_aggregated_htlc_mincorrectly includes themin_final_cltv_expiry_deltaparameter in its call to thecompute_payinfofunction. This ensures the test case aligns with the updated logic for calculating the CLTV expiry delta.- 468-468: The test case
aggregated_htlc_minhas been updated to include themin_final_cltv_expiry_deltaparameter when calling thecompute_payinfofunction. This change ensures the test accurately reflects the updated logic for handling themin_final_cltv_delta.- 519-519: The test case
aggregated_htlc_maxcorrectly passes themin_final_cltv_expiry_deltaparameter to thecompute_payinfofunction. This inclusion is crucial for testing the correct aggregation of the CLTV deltas with the updated logic.lightning/src/ln/blinded_payment_tests.rs (3)
- 103-104: The usage of
TEST_FINAL_CLTVinBlindedPath::one_hop_for_paymentcall ensures that the test accurately reflects the handling ofmin_final_cltv_delta. This change aligns with the PR's objective to correct the oversight inmin_final_cltv_deltahandling.- 145-146: Similar to the previous comment, the usage of
TEST_FINAL_CLTVin anotherBlindedPath::one_hop_for_paymentcall is consistent with the PR's objective and enhances the test's accuracy in reflecting the handling ofmin_final_cltv_delta.- 694-694: The assertion to check the
cltv_expiryvalue in thedo_multi_hop_receiver_failfunction with theProcessPendingHTLCsCheckcase is a critical addition. It ensures that thecltv_expiryis calculated correctly, considering theTEST_FINAL_CLTV. This change directly addresses the oversight in handlingmin_final_cltv_deltaand is essential for validating the correctness of the implementation.lightning/src/routing/router.rs (3)
- 19-19: The addition of
MIN_FINAL_CLTV_EXPIRY_DELTAto the import statement fromchannelmanageris correctly aligned with the PR objectives to include themin_final_cltv_deltain the calculation ofcltv_expiry_delta. This change ensures that the necessary constant is available for use in therouter.rsfile, which is crucial for the correct implementation of the route blinding feature as described.- 142-143: The inclusion of
MIN_FINAL_CLTV_EXPIRY_DELTAas an argument in theBlindedPath::new_for_paymentmethod call is consistent with the PR's goal to correct the aggregatedcltv_expiry_deltacalculation. This ensures that the minimum final CLTV expiry delta is considered in the construction of a blinded path for payments, aligning with the intended functionality of route blinding. The change is logically placed and correctly implemented.- 154-155: Similarly, the use of
MIN_FINAL_CLTV_EXPIRY_DELTAas an argument in theBlindedPath::one_hop_for_paymentmethod call is in line with the PR's objectives. This modification ensures that the route blinding feature correctly incorporates the minimum final CLTV expiry delta in its calculations, enhancing the privacy and efficiency of routing in the Lightning Network. The implementation here is correct and follows the intended logic.
TheBlueMatt
previously approved these changes
Feb 7, 2024
There was a problem hiding this comment.
Didn't compare it to the spec changes but this makes sense to me. @jkczyz ?
Currently we are not including this value in the aggregated CLTV delta, which is wrong.
The spec was previously missing this requirement.
valentinewallace
force-pushed
the
2024-01-blinded-payinfo-min-final-cltv
branch
from
9339454 to
fff1aa7
Compare
|
Rebased due to conflict and removed the last commit which was done in #2847. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/blinded_path/mod.rs (2 hunks)
- lightning/src/blinded_path/payment.rs (7 hunks)
- lightning/src/ln/blinded_payment_tests.rs (4 hunks)
- lightning/src/routing/router.rs (3 hunks)
Additional comments: 10
lightning/src/blinded_path/mod.rs (2)
- 88-96: The addition of
min_final_cltv_expiry_deltatoone_hop_for_paymentand its propagation tonew_for_paymentcorrectly implements the updated specification requirements. Ensure that all calls to these functions throughout the codebase are updated to include this new parameter.Verification successful
The examination of the specific lines where
one_hop_for_paymentandnew_for_paymentare called within the codebase reveals that themin_final_cltv_expiry_deltaparameter is indeed included in all calls to these functions. This confirms that the changes made to includemin_final_cltv_expiry_deltain the function signatures are correctly implemented across the codebase, aligning with the updated specification requirements for route blinding in the Lightning Network.
- In
router.rs, both functions are called withMIN_FINAL_CLTV_EXPIRY_DELTAas the parameter formin_final_cltv_expiry_delta.- In
blinded_payment_tests.rs, the test cases useTEST_FINAL_CLTV as u16for themin_final_cltv_expiry_deltaparameter, which is appropriate for testing purposes.- The internal call within
mod.rsitself correctly passesmin_final_cltv_expiry_deltaas a parameter.This confirms that the review comment is accurate and the implementation adheres to the updated specifications, ensuring that all calls to these functions throughout the codebase are updated to include the new parameter.
* 111-119: The inclusion of `min_final_cltv_expiry_delta` in `new_for_payment` and its use in `compute_payinfo` is correctly implemented. This change ensures the accurate calculation of `cltv_expiry_delta` for blinded payments. Verify that the handling of this parameter does not introduce any integer overflows or underflows in `compute_payinfo`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to one_hop_for_payment and new_for_payment without the new parameter. rg --type rust 'one_hop_for_payment\(' rg --type rust 'new_for_payment\('Length of output: 538
Script:
#!/bin/bash # Extract the specific lines of code for one_hop_for_payment and new_for_payment function calls rg --type rust -C 3 'one_hop_for_payment\(' lightning/src/routing/router.rs rg --type rust -C 3 'one_hop_for_payment\(' lightning/src/ln/blinded_payment_tests.rs rg --type rust -C 3 'new_for_payment\(' lightning/src/routing/router.rs rg --type rust -C 3 'new_for_payment\(' lightning/src/ln/blinded_payment_tests.rs rg --type rust -C 3 'new_for_payment\(' lightning/src/blinded_path/mod.rsLength of output: 1957
lightning/src/blinded_path/payment.rs (1)
- 215-226: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [218-347]
The update to
compute_payinfoto includemin_final_cltv_expiry_deltaand its use in initializingcltv_expiry_deltais correctly implemented. The tests have been appropriately updated to pass this new parameter, validating the correct handling ofmin_final_cltv_expiry_deltain various scenarios. Ensure comprehensive test coverage for edge cases related tomin_final_cltv_expiry_delta, such as extremely high or low values.lightning/src/ln/blinded_payment_tests.rs (4)
- 64-64:
TEST_FINAL_CLTVis added as a parameter toBlindedPath::new_for_paymentfunction call. EnsureTEST_FINAL_CLTVis properly defined and initialized in this file or imported from a module where it is defined.- 103-104:
TEST_FINAL_CLTVis added as a parameter toBlindedPath::one_hop_for_paymentfunction calls. Verify thatTEST_FINAL_CLTVis correctly used and matches the expected type and value as per the updated specification.- 145-146: Similar to previous comments,
TEST_FINAL_CLTVis added to anotherBlindedPath::one_hop_for_paymentcall. Confirm consistency and correctness in its application across all test cases.- 700-700: The assertion on line 700 checks the
cltv_expiryagainst a calculated value includingTEST_FINAL_CLTV. This is part of a test case designed to simulate a failure scenario. Ensure that the logic for calculating the expectedcltv_expiryvalue is correct and aligns with the intended test scenario.lightning/src/routing/router.rs (3)
- 17-17: Ensure that
MIN_FINAL_CLTV_EXPIRY_DELTAis used consistently across the file and that its import aligns with the project's import organization conventions.- 137-138: Verify that the addition of
MIN_FINAL_CLTV_EXPIRY_DELTAas a parameter toBlindedPath::new_for_paymentaligns with the method's updated signature and that the value passed is appropriate for the context.- 148-150: Confirm that
MIN_FINAL_CLTV_EXPIRY_DELTAis correctly applied inBlindedPath::one_hop_for_paymentand that its inclusion does not introduce any logical errors or inconsistencies.
jkczyz
approved these changes
Feb 7, 2024
TheBlueMatt
approved these changes
Feb 7, 2024
k0k0ne
pushed a commit
to bitlightlabs/rust-lightning
that referenced
this pull request
Sep 30, 2024
v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we were not including the
min_final_cltv_deltain the aggregatedBlindedPayInfo::cltv_expiry_delta.This requirement was missing from the spec. It's now been added in lightning/bolts#1131.
IIUC, the spec is currently missing this requirement -- spec authors may have thought that it was implied. Another look at t-bast's gist https://gist.github.com/t-bast/b1371d357a2c5f3e8c09514a62db7079 tipped me off that it was missing in LDK.