From eb5198ac34fdffb6643b519192d7e51367e92943 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 31 Jan 2025 17:33:26 +0000 Subject: [PATCH 1/4] Avoid parsing `PublicKey`s when applying an unsigned chan update `PublicKey` parsing is relatively expensive as we have to check if the point is actually on the curve. To avoid it, our `NetworkGraph` uses `NodeId`s which don't have the validity requirement. Sadly, we were always parsing the broadcasting node's `PublicKey` from the `node_id` in the network graph whenever we see an update for that channel, whether we have a corresponding signature or not. Here we fix this, only parsing the public key (and hashing the message) if we're going to check a signature. --- lightning-background-processor/src/lib.rs | 4 ++-- lightning/src/routing/gossip.rs | 27 ++++++++++++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index adcae564f56..ef889d4e80f 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -2379,8 +2379,8 @@ mod tests { 42, 53, features, - $nodes[0].node.get_our_node_id(), - $nodes[1].node.get_our_node_id(), + $nodes[0].node.get_our_node_id().into(), + $nodes[1].node.get_our_node_id().into(), ) .expect("Failed to update channel from partial announcement"); let original_graph_description = $nodes[0].network_graph.to_string(); diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index c552005d9ca..e3a1e9d0878 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -2537,7 +2537,7 @@ where } }; - let node_pubkey; + let mut node_pubkey = None; { let channels = self.channels.read().unwrap(); match channels.get(&msg.short_channel_id) { @@ -2556,16 +2556,31 @@ where } else { channel.node_one.as_slice() }; - node_pubkey = PublicKey::from_slice(node_id).map_err(|_| LightningError { - err: "Couldn't parse source node pubkey".to_owned(), - action: ErrorAction::IgnoreAndLog(Level::Debug), - })?; + if sig.is_some() { + // PublicKey parsing isn't entirely trivial as it requires that we check + // that the provided point is on the curve. Thus, if we don't have a + // signature to verify, we want to skip the parsing step entirely. + // This represents a substantial speedup in applying RGS snapshots. + node_pubkey = + Some(PublicKey::from_slice(node_id).map_err(|_| LightningError { + err: "Couldn't parse source node pubkey".to_owned(), + action: ErrorAction::IgnoreAndLog(Level::Debug), + })?); + } }, } } - let msg_hash = hash_to_message!(&message_sha256d_hash(&msg)[..]); if let Some(sig) = sig { + let msg_hash = hash_to_message!(&message_sha256d_hash(&msg)[..]); + let node_pubkey = if let Some(pubkey) = node_pubkey { + pubkey + } else { + debug_assert!(false, "node_pubkey should have been decoded above"); + let err = "node_pubkey wasn't decoded but we need it to check a sig".to_owned(); + let action = ErrorAction::IgnoreAndLog(Level::Error); + return Err(LightningError { err, action }); + }; secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &node_pubkey, "channel_update"); } From c1bb1dfd8d70677e8368348e6daf0a9f002fe98b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 31 Jan 2025 15:32:25 +0000 Subject: [PATCH 2/4] Avoid unnecessary `PublicKey` parsing in RGS application `PublicKey` parsing is relatively expensive as we have to check if the point is actually on the curve. To avoid it, our `NetworkGraph` uses `NodeId`s which don't have the validity requirement. Here, we take advantage of that in RGS application to avoid parsing `PublicKey`s, improving performance. --- lightning-rapid-gossip-sync/src/processing.rs | 8 +++----- lightning/src/routing/gossip.rs | 10 ++++------ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lightning-rapid-gossip-sync/src/processing.rs b/lightning-rapid-gossip-sync/src/processing.rs index c61187fcfff..708d54160eb 100644 --- a/lightning-rapid-gossip-sync/src/processing.rs +++ b/lightning-rapid-gossip-sync/src/processing.rs @@ -3,7 +3,6 @@ use core::ops::Deref; use core::sync::atomic::Ordering; use bitcoin::constants::ChainHash; -use bitcoin::secp256k1::PublicKey; use lightning::io; use lightning::ln::msgs::{ @@ -117,7 +116,7 @@ where }; let node_id_count: u32 = Readable::read(read_cursor)?; - let mut node_ids: Vec = Vec::with_capacity(core::cmp::min( + let mut node_ids: Vec = Vec::with_capacity(core::cmp::min( node_id_count, MAX_INITIAL_NODE_ID_VECTOR_CAPACITY, ) as usize); @@ -154,9 +153,8 @@ where let key_parity = node_detail_flag & 0b_0000_0011; pubkey_bytes[0] = key_parity; - let current_pubkey = PublicKey::from_slice(&pubkey_bytes)?; - let current_node_id = NodeId::from_pubkey(¤t_pubkey); - node_ids.push(current_pubkey); + let current_node_id = NodeId::from_slice(&pubkey_bytes)?; + node_ids.push(current_node_id); if is_reminder || has_address_details || feature_detail_marker > 0 { let mut synthetic_node_announcement = UnsignedNodeAnnouncement { diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index e3a1e9d0878..757a28864a2 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -1992,8 +1992,8 @@ where /// /// All other parameters as used in [`msgs::UnsignedChannelAnnouncement`] fields. pub fn add_channel_from_partial_announcement( - &self, short_channel_id: u64, timestamp: u64, features: ChannelFeatures, - node_id_1: PublicKey, node_id_2: PublicKey, + &self, short_channel_id: u64, timestamp: u64, features: ChannelFeatures, node_id_1: NodeId, + node_id_2: NodeId, ) -> Result<(), LightningError> { if node_id_1 == node_id_2 { return Err(LightningError { @@ -2002,13 +2002,11 @@ where }); }; - let node_1 = NodeId::from_pubkey(&node_id_1); - let node_2 = NodeId::from_pubkey(&node_id_2); let channel_info = ChannelInfo { features, - node_one: node_1.clone(), + node_one: node_id_1, one_to_two: None, - node_two: node_2.clone(), + node_two: node_id_2, two_to_one: None, capacity_sats: None, announcement_message: None, From c46f299acd6ca5d9527968639fbb43b61ea98820 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 31 Jan 2025 17:42:51 +0000 Subject: [PATCH 3/4] Properly pre-allocate `NetworkGraph` channel/node maps When we build a new `NetworkGraph` from empty, we're generally doing an initial startup and will be syncing the graph very soon. Using an initially-empty `IndexedMap` for the `channels` and `nodes` results in quite some memory churn, with the initial RGS application benchmark showing 15% of its time in pagefault handling alone (i.e. allocating new memory from the OS, let alone the 23% of time in `memmove`). Further, when deserializing a `NetworkGraph`, we'd swapped the expected node and channel count constants, leaving the node map too small and causing map doubling as we read entries from disk. Finally, when deserializing, allocating only exactly the amount of map entries we need is likely to lead to at least one doubling, so we're better off just over-estimating the number of nodes and channels and allocating what we want. Here we just always allocate `channels` and `nodes` based on constants, leading to a 20%-ish speedup in the initial RGS application benchmark. --- lightning/src/routing/gossip.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 757a28864a2..07b67b1f594 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -1660,8 +1660,7 @@ where let chain_hash: ChainHash = Readable::read(reader)?; let channels_count: u64 = Readable::read(reader)?; - // In Nov, 2023 there were about 15,000 nodes; we cap allocations to 1.5x that. - let mut channels = IndexedMap::with_capacity(cmp::min(channels_count as usize, 22500)); + let mut channels = IndexedMap::with_capacity(CHAN_COUNT_ESTIMATE); for _ in 0..channels_count { let chan_id: u64 = Readable::read(reader)?; let chan_info: ChannelInfo = Readable::read(reader)?; @@ -1673,8 +1672,7 @@ where if nodes_count > u32::max_value() as u64 / 2 { return Err(DecodeError::InvalidValue); } - // In Nov, 2023 there were about 69K channels; we cap allocations to 1.5x that. - let mut nodes = IndexedMap::with_capacity(cmp::min(nodes_count as usize, 103500)); + let mut nodes = IndexedMap::with_capacity(NODE_COUNT_ESTIMATE); for i in 0..nodes_count { let node_id = Readable::read(reader)?; let mut node_info: NodeInfo = Readable::read(reader)?; @@ -1750,6 +1748,15 @@ where } } +// In Jan, 2025 there were about 49K channels. +// We over-allocate by a bit because 20% more is better than the double we get if we're slightly +// too low +const CHAN_COUNT_ESTIMATE: usize = 60_000; +// In Jan, 2025 there were about 15K nodes +// We over-allocate by a bit because 33% more is better than the double we get if we're slightly +// too low +const NODE_COUNT_ESTIMATE: usize = 20_000; + impl NetworkGraph where L::Target: Logger, @@ -1760,8 +1767,8 @@ where secp_ctx: Secp256k1::verification_only(), chain_hash: ChainHash::using_genesis_block(network), logger, - channels: RwLock::new(IndexedMap::new()), - nodes: RwLock::new(IndexedMap::new()), + channels: RwLock::new(IndexedMap::with_capacity(CHAN_COUNT_ESTIMATE)), + nodes: RwLock::new(IndexedMap::with_capacity(NODE_COUNT_ESTIMATE)), next_node_counter: AtomicUsize::new(0), removed_node_counters: Mutex::new(Vec::new()), last_rapid_gossip_sync_timestamp: Mutex::new(None), From 0ff7fba4ec0141932ed715a611b5a4d077d4d8a2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 2 Feb 2025 20:01:42 +0000 Subject: [PATCH 4/4] Better describe the `NodeId` type and why it exists --- lightning/src/routing/gossip.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 07b67b1f594..2870f4c351c 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -72,7 +72,12 @@ const MAX_EXCESS_BYTES_FOR_RELAY: usize = 1024; /// This value ensures a reply fits within the 65k payload limit and is consistent with other implementations. const MAX_SCIDS_PER_REPLY: usize = 8000; -/// Represents the compressed public key of a node +/// A compressed pubkey which a node uses to sign announcements and decode HTLCs routed through it. +/// +/// This type stores a simple byte array which is not checked for validity (i.e. that it describes +/// a point which lies on the secp256k1 curve), unlike [`PublicKey`], as validity checking would +/// otherwise represent a large portion of [`NetworkGraph`] deserialization time (and RGS +/// application). #[derive(Clone, Copy, PartialEq, Eq)] pub struct NodeId([u8; PUBLIC_KEY_SIZE]);