-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prefer non-Tor nodes when creating blinded paths #2911
Prefer non-Tor nodes when creating blinded paths #2911
Conversation
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_paths
is 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
DefaultMessageRouter
to 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_only
correctly checks if all announced addresses for a node are Tor addresses, returningtrue
if 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_tor
method correctly identifies whether aSocketAddress
is a Tor address based on its variant. This method is straightforward and correctly handles all defined variants ofSocketAddress
, returningtrue
for bothOnionV2
andOnionV3
addresses, andfalse
otherwise.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
};
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_only
method 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Changes seem small enough and greatly tested, so I'll go ahead and merge this.
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. |
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