Prefer non-Tor nodes when creating blinded paths #2911
Merged
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
Add a method to NodeInfo to determine if the node has only announced Tor addresses. Useful for preferring blinded paths that don't use Tor for better reliability and improved latency.
Tor nodes can have high latency which can have a detrimental effect on onion message reliability. Prefer using nodes that aren't Tor-only when creating blinded paths both in offers and in onion message reply paths.
WalkthroughThe recent updates in the codebase introduce enhancements in network address handling, testing for offers, message filtering and sorting, and gossip protocol improvements. A notable change is the addition of methods to identify Tor addresses and prefer non-Tor nodes in blinded paths, aligning with objectives to improve privacy and reduce latency in the network. These changes collectively aim to refine the node selection process, message routing, and offer testing, contributing to the network's efficiency and privacy. Changes
Assessment against linked issues
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 (4)
- lightning/src/ln/msgs.rs (1 hunks)
- lightning/src/ln/offers_tests.rs (3 hunks)
- lightning/src/onion_message/messenger.rs (1 hunks)
- lightning/src/routing/gossip.rs (3 hunks)
Additional comments: 5
lightning/src/ln/offers_tests.rs (2)
- 48-57: The addition of new imports enhances the file's capabilities for handling messages, routing, and signing. Ensure that all newly imported modules are used within the file to avoid unnecessary imports, which can lead to increased compilation time and binary size.
- 214-264: The test
prefers_non_tor_nodes_in_blinded_pathsis comprehensive and effectively validates the preference for non-Tor nodes in blinded paths. It's well-structured and follows best practices for test writing. However, consider adding comments within the test to explain the setup and assertions for future maintainability.lightning/src/onion_message/messenger.rs (1)
- 361-371: The implementation of filtering and sorting logic in the
DefaultMessageRouterto prefer non-Tor nodes is a significant change aimed at improving the reliability of onion messages. This change is well-aligned with the PR's objectives. However, it's important to ensure that these changes are thoroughly tested, especially in scenarios where the mix of Tor and non-Tor nodes might affect the path selection in unexpected ways.lightning/src/routing/gossip.rs (1)
- 1234-1244: The method
is_tor_onlycorrectly checks if all announced addresses for a node are Tor addresses, returningtrueif so. This method enhances the network graph's ability to categorize nodes based on their address types, which could be useful for routing decisions or privacy considerations.lightning/src/ln/msgs.rs (1)
- 846-855: The
is_tormethod correctly identifies whether aSocketAddressis a Tor address based on its variant. This method is straightforward and correctly handles all defined variants ofSocketAddress, returningtruefor bothOnionV2andOnionV3addresses, andfalseotherwise.
Comment on lines
+103
to
+132
| fn announce_node_address<'a, 'b, 'c>( | ||
| node: &Node<'a, 'b, 'c>, peers: &[&Node<'a, 'b, 'c>], address: SocketAddress, | ||
| ) { | ||
| let features = node.onion_messenger.provided_node_features() | ||
| | node.gossip_sync.provided_node_features(); | ||
| let rgb = [0u8; 3]; | ||
| let announcement = UnsignedNodeAnnouncement { | ||
| features, | ||
| timestamp: 1000, | ||
| node_id: NodeId::from_pubkey(&node.keys_manager.get_node_id(Recipient::Node).unwrap()), | ||
| rgb, | ||
| alias: NodeAlias([0u8; 32]), | ||
| addresses: vec![address], | ||
| excess_address_data: Vec::new(), | ||
| excess_data: Vec::new(), | ||
| }; | ||
| let signature = node.keys_manager.sign_gossip_message( | ||
| UnsignedGossipMessage::NodeAnnouncement(&announcement) | ||
| ).unwrap(); | ||
|
|
||
| let msg = NodeAnnouncement { | ||
| signature, | ||
| contents: announcement | ||
| }; | ||
|
|
||
| node.gossip_sync.handle_node_announcement(&msg).unwrap(); | ||
| for peer in peers { | ||
| peer.gossip_sync.handle_node_announcement(&msg).unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The function announce_node_address is well-structured and follows Rust's idiomatic practices. It correctly constructs a NodeAnnouncement message and disseminates it to the specified peers. However, consider adding error handling for the unwrap calls to prevent potential panics in production code.
// Replace unwrap calls with error handling
let node_id = match node.keys_manager.get_node_id(Recipient::Node) {
Ok(id) => NodeId::from_pubkey(&id),
Err(e) => return Err(e), // Adjust the function signature to return a Result
};
let signature = match node.keys_manager.sign_gossip_message(UnsignedGossipMessage::NodeAnnouncement(&announcement)) {
Ok(sig) => sig,
Err(e) => return Err(e), // Adjust the function signature to return a Result
};
Comment on lines
+361
to
+371
| let mut peer_info = peers.iter() | ||
| // Limit to peers with announced channels | ||
| .filter(|pubkey| | ||
| .filter_map(|pubkey| | ||
| network_graph | ||
| .node(&NodeId::from_pubkey(pubkey)) | ||
| .map(|info| &info.channels[..]) | ||
| .map(|channels| channels.len() >= MIN_PEER_CHANNELS) | ||
| .unwrap_or(false) | ||
| .filter(|info| info.channels.len() >= MIN_PEER_CHANNELS) | ||
| .map(|info| (*pubkey, info.is_tor_only())) | ||
| ) | ||
| .map(|pubkey| vec![*pubkey, recipient]) | ||
| .collect::<Vec<_>>(); | ||
| peer_info.sort_unstable_by(|(_, a_tor_only), (_, b_tor_only)| a_tor_only.cmp(b_tor_only)); | ||
|
|
There was a problem hiding this comment.
The logic for filtering and sorting peers based on their channel information and Tor status has been updated. While the approach of using filter_map and sort_unstable_by is efficient for this purpose, there are a few considerations:
- Ensure that the
is_tor_onlymethod accurately reflects whether a node exclusively uses Tor addresses, as this directly impacts the filtering logic. - The sorting based on Tor status (
a_tor_only.cmp(b_tor_only)) prioritizes non-Tor nodes, aligning with the PR's objectives. However, it's important to verify that this sorting criterion effectively contributes to the reliability of onion messages without introducing biases that could affect network diversity or privacy. - Consider adding comments to explain the rationale behind preferring non-Tor nodes and how it relates to the overall goal of enhancing onion message reliability.
Consider enhancing the documentation within this code segment to explain the rationale behind the filtering and sorting logic, especially for future maintainers or contributors who may not be familiar with the specific objectives of these changes.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2911 +/- ##
==========================================
+ Coverage 89.11% 89.13% +0.02%
==========================================
Files 117 117
Lines 94657 94853 +196
Branches 94657 94853 +196
==========================================
+ Hits 84350 84551 +201
+ Misses 7826 7822 -4
+ Partials 2481 2480 -1 ☔ View full report in Codecov by Sentry. |
tnull
approved these changes
Feb 28, 2024
|
Currently to get a DC reliably we really want to ensure the intro node has public ips, so if we're a public node with public IPs and our peers are all tor-only we should still prefer to use a one-hop path. I'm not sure if its a huge deal, but I think there the reliability win would be worth it. |
Yeah, was going to ask if you prefer filtering or sorting. We also aren't filtering out nodes without any public IPs currently. I'll do a follow-up as I'm also updating to prefer more well-connected nodes. |
Although, for RGS users this would mean possibly failing to create a path. |
|
Right, can't break RGS entirely. |
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.
Tor nodes can have high latency which can have a detrimental effect on onion message reliability. Prefer using nodes that aren't Tor-only when creating blinded paths both in offers and in onion message reply paths.
Fixes #2893