Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prefer non-Tor nodes when creating blinded paths #2911

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Prefer non-Tor nodes when creating blinded paths #2911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
093ceff
Add NodeInfo::is_tor_only
jkczyz Feb 27, 2024
b954748
Prefer non-Tor nodes when creating blinded paths
jkczyz Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lightning/src/ln/msgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,16 @@ impl SocketAddress {
/// This maximum length is reached by a hostname address descriptor:
/// a hostname with a maximum length of 255, its 1-byte length and a 2-byte port.
pub(crate) const MAX_LEN: u16 = 258;

pub(crate) fn is_tor(&self) -> bool {
match self {
&SocketAddress::TcpIpV4 {..} => false,
&SocketAddress::TcpIpV6 {..} => false,
&SocketAddress::OnionV2(_) => true,
&SocketAddress::OnionV3 {..} => true,
&SocketAddress::Hostname {..} => false,
}
}
}

impl Writeable for SocketAddress {
Expand Down
87 changes: 86 additions & 1 deletion lightning/src/ln/offers_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,16 @@ use crate::blinded_path::BlindedPath;
use crate::events::{Event, MessageSendEventsProvider, PaymentPurpose};
use crate::ln::channelmanager::{PaymentId, RecentPaymentDetails, Retry, self};
use crate::ln::functional_test_utils::*;
use crate::ln::msgs::{ChannelMessageHandler, Init, OnionMessage, OnionMessageHandler};
use crate::ln::msgs::{ChannelMessageHandler, Init, NodeAnnouncement, OnionMessage, OnionMessageHandler, RoutingMessageHandler, SocketAddress, UnsignedGossipMessage, UnsignedNodeAnnouncement};
use crate::offers::invoice::Bolt12Invoice;
use crate::offers::invoice_error::InvoiceError;
use crate::offers::invoice_request::InvoiceRequest;
use crate::offers::parse::Bolt12SemanticError;
use crate::onion_message::messenger::PeeledOnion;
use crate::onion_message::offers::OffersMessage;
use crate::onion_message::packet::ParsedOnionMessageContents;
use crate::routing::gossip::{NodeAlias, NodeId};
use crate::sign::{NodeSigner, Recipient};

use crate::prelude::*;

Expand Down Expand Up @@ -98,6 +100,37 @@ fn disconnect_peers<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, peers: &[&Node<'a, 'b
}
}

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();
}
}
Comment on lines +103 to +132
Copy link

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
};


fn route_bolt12_payment<'a, 'b, 'c>(
node: &Node<'a, 'b, 'c>, path: &[&Node<'a, 'b, 'c>], invoice: &Bolt12Invoice
) {
Expand Down Expand Up @@ -178,6 +211,58 @@ fn extract_invoice_error<'a, 'b, 'c>(
}
}

/// Checks that blinded paths without Tor-only nodes are preferred when constructing an offer.
#[test]
fn prefers_non_tor_nodes_in_blinded_paths() {
let mut accept_forward_cfg = test_default_channel_config();
accept_forward_cfg.accept_forwards_to_priv_channels = true;

let mut features = channelmanager::provided_init_features(&accept_forward_cfg);
features.set_onion_messages_optional();
features.set_route_blinding_optional();

let chanmon_cfgs = create_chanmon_cfgs(6);
let node_cfgs = create_node_cfgs(6, &chanmon_cfgs);

*node_cfgs[1].override_init_features.borrow_mut() = Some(features);

let node_chanmgrs = create_node_chanmgrs(
6, &node_cfgs, &[None, Some(accept_forward_cfg), None, None, None, None]
);
let nodes = create_network(6, &node_cfgs, &node_chanmgrs);

create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000);
create_unannounced_chan_between_nodes_with_value(&nodes, 2, 3, 10_000_000, 1_000_000_000);
create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 1_000_000_000);
create_announced_chan_between_nodes_with_value(&nodes, 1, 4, 10_000_000, 1_000_000_000);
create_announced_chan_between_nodes_with_value(&nodes, 1, 5, 10_000_000, 1_000_000_000);
create_announced_chan_between_nodes_with_value(&nodes, 2, 4, 10_000_000, 1_000_000_000);
create_announced_chan_between_nodes_with_value(&nodes, 2, 5, 10_000_000, 1_000_000_000);

// Add an extra channel so that more than one of Bob's peers have MIN_PEER_CHANNELS.
create_announced_chan_between_nodes_with_value(&nodes, 4, 5, 10_000_000, 1_000_000_000);

let (alice, bob, charlie, david) = (&nodes[0], &nodes[1], &nodes[2], &nodes[3]);
let bob_id = bob.node.get_our_node_id();
let charlie_id = charlie.node.get_our_node_id();

disconnect_peers(alice, &[charlie, david, &nodes[4], &nodes[5]]);
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);

let offer = bob.node
.create_offer_builder("coffee".to_string()).unwrap()
.amount_msats(10_000_000)
.build().unwrap();
assert_ne!(offer.signing_pubkey(), bob_id);
assert!(!offer.paths().is_empty());
for path in offer.paths() {
assert_ne!(path.introduction_node_id, charlie_id);
}
}

/// Checks that an offer can be paid through blinded paths and that ephemeral pubkeys are used
/// rather than exposing a node's pubkey.
#[test]
Expand Down
15 changes: 9 additions & 6 deletions lightning/src/onion_message/messenger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,16 +358,19 @@ where
const MIN_PEER_CHANNELS: usize = 3;

let network_graph = self.network_graph.deref().read_only();
let paths = peers.iter()
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));

Comment on lines +361 to +371
Copy link

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.

let paths = peer_info.into_iter()
.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<_>, _>>();
Expand Down
121 changes: 120 additions & 1 deletion lightning/src/routing/gossip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,18 @@ pub struct NodeInfo {
pub announcement_info: Option<NodeAnnouncementInfo>
}

impl NodeInfo {
/// Returns whether the node has only announced Tor addresses.
pub fn is_tor_only(&self) -> bool {
self.announcement_info
.as_ref()
.map(|info| info.addresses())
.and_then(|addresses| (!addresses.is_empty()).then(|| addresses))
.map(|addresses| addresses.iter().all(|address| address.is_tor()))
.unwrap_or(false)
}
}

impl fmt::Display for NodeInfo {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
write!(f, " channels: {:?}, announcement_info: {:?}",
Expand Down Expand Up @@ -2089,14 +2101,15 @@ pub(crate) mod tests {
use crate::ln::chan_utils::make_funding_redeemscript;
#[cfg(feature = "std")]
use crate::ln::features::InitFeatures;
use crate::ln::msgs::SocketAddress;
use crate::routing::gossip::{P2PGossipSync, NetworkGraph, NetworkUpdate, NodeAlias, MAX_EXCESS_BYTES_FOR_RELAY, NodeId, RoutingFees, ChannelUpdateInfo, ChannelInfo, NodeAnnouncementInfo, NodeInfo};
use crate::routing::utxo::{UtxoLookupError, UtxoResult};
use crate::ln::msgs::{RoutingMessageHandler, UnsignedNodeAnnouncement, NodeAnnouncement,
UnsignedChannelAnnouncement, ChannelAnnouncement, UnsignedChannelUpdate, ChannelUpdate,
ReplyChannelRange, QueryChannelRange, QueryShortChannelIds, MAX_VALUE_MSAT};
use crate::util::config::UserConfig;
use crate::util::test_utils;
use crate::util::ser::{ReadableArgs, Readable, Writeable};
use crate::util::ser::{Hostname, ReadableArgs, Readable, Writeable};
use crate::util::scid_utils::scid_from_parts;

use crate::routing::gossip::REMOVED_ENTRIES_TRACKING_AGE_LIMIT_SECS;
Expand Down Expand Up @@ -3474,6 +3487,112 @@ pub(crate) mod tests {
let node_id = NodeId([42; 33]);
assert_eq!(format!("{}", &node_id), "2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a");
}

#[test]
fn is_tor_only_node() {
let network_graph = create_network_graph();
let (secp_ctx, gossip_sync) = create_gossip_sync(&network_graph);

let node_1_privkey = &SecretKey::from_slice(&[42; 32]).unwrap();
let node_2_privkey = &SecretKey::from_slice(&[41; 32]).unwrap();
let node_1_id = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_privkey));

let announcement = get_signed_channel_announcement(|_| {}, node_1_privkey, node_2_privkey, &secp_ctx);
gossip_sync.handle_channel_announcement(&announcement).unwrap();

let tcp_ip_v4 = SocketAddress::TcpIpV4 {
addr: [255, 254, 253, 252],
port: 9735
};
let tcp_ip_v6 = SocketAddress::TcpIpV6 {
addr: [255, 254, 253, 252, 251, 250, 249, 248, 247, 246, 245, 244, 243, 242, 241, 240],
port: 9735
};
let onion_v2 = SocketAddress::OnionV2([255, 254, 253, 252, 251, 250, 249, 248, 247, 246, 38, 7]);
let onion_v3 = SocketAddress::OnionV3 {
ed25519_pubkey: [255, 254, 253, 252, 251, 250, 249, 248, 247, 246, 245, 244, 243, 242, 241, 240, 239, 238, 237, 236, 235, 234, 233, 232, 231, 230, 229, 228, 227, 226, 225, 224],
checksum: 32,
version: 16,
port: 9735
};
let hostname = SocketAddress::Hostname {
hostname: Hostname::try_from(String::from("host")).unwrap(),
port: 9735,
};

assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(|_| {}, node_1_privkey, &secp_ctx);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(
|announcement| {
announcement.addresses = vec![
tcp_ip_v4.clone(), tcp_ip_v6.clone(), onion_v2.clone(), onion_v3.clone(),
hostname.clone()
];
announcement.timestamp += 1000;
},
node_1_privkey, &secp_ctx
);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(
|announcement| {
announcement.addresses = vec![
tcp_ip_v4.clone(), tcp_ip_v6.clone(), onion_v2.clone(), onion_v3.clone()
];
announcement.timestamp += 2000;
},
node_1_privkey, &secp_ctx
);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(
|announcement| {
announcement.addresses = vec![
tcp_ip_v6.clone(), onion_v2.clone(), onion_v3.clone()
];
announcement.timestamp += 3000;
},
node_1_privkey, &secp_ctx
);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(
|announcement| {
announcement.addresses = vec![onion_v2.clone(), onion_v3.clone()];
announcement.timestamp += 4000;
},
node_1_privkey, &secp_ctx
);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(
|announcement| {
announcement.addresses = vec![onion_v2.clone()];
announcement.timestamp += 5000;
},
node_1_privkey, &secp_ctx
);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());

let announcement = get_signed_node_announcement(
|announcement| {
announcement.addresses = vec![tcp_ip_v4.clone()];
announcement.timestamp += 6000;
},
node_1_privkey, &secp_ctx
);
gossip_sync.handle_node_announcement(&announcement).unwrap();
assert!(!network_graph.read_only().node(&node_1_id).unwrap().is_tor_only());
}
}

#[cfg(ldk_bench)]
Expand Down
Loading