From 0b7838b59de357bce02d86fc840a3906be8d189f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Sep 2024 23:29:07 +0000 Subject: [PATCH 1/2] Refactor `channel_update` processing logic into local fns In the next commit we'll move to checking `channel_update`s in three steps - first we check if the `channel_update` is new and the latest for a channel, then we check the signature, and finally we update our local state. This allows us to avoid holding a lock on `NetworkGraph::channels` while validating the message signature. Here we do a quick prefactor to make that simpler - moving the validation logic of `channel_update` that we'll do in step one (and repeat in step three) into a local function. We also take this opportunity to do one static check unlocked which we had been doing while holding a `channel` lock. --- lightning/src/routing/gossip.rs | 76 ++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 34 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 4bcb8b859c6..4185ca05ef9 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -2253,6 +2253,47 @@ impl NetworkGraph where L::Target: Logger { msg.timestamp ); + if msg.htlc_maximum_msat > MAX_VALUE_MSAT { + return Err(LightningError{err: + "htlc_maximum_msat is larger than maximum possible msats".to_owned(), + action: ErrorAction::IgnoreError}); + } + + let check_update_latest = |target: &Option| -> Result<(), LightningError> { + if let Some(existing_chan_info) = target { + // The timestamp field is somewhat of a misnomer - the BOLTs use it to + // order updates to ensure you always have the latest one, only + // suggesting that it be at least the current time. For + // channel_updates specifically, the BOLTs discuss the possibility of + // pruning based on the timestamp field being more than two weeks old, + // but only in the non-normative section. + if existing_chan_info.last_update > msg.timestamp { + return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); + } else if existing_chan_info.last_update == msg.timestamp { + return Err(LightningError{err: "Update had same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); + } + } + Ok(()) + }; + + let check_msg_sanity = |channel: &ChannelInfo| -> Result<(), LightningError> { + if let Some(capacity_sats) = channel.capacity_sats { + // It's possible channel capacity is available now, although it wasn't available at announcement (so the field is None). + // Don't query UTXO set here to reduce DoS risks. + if capacity_sats > MAX_VALUE_MSAT / 1000 || msg.htlc_maximum_msat > capacity_sats * 1000 { + return Err(LightningError{err: + "htlc_maximum_msat is larger than channel capacity or capacity is bogus".to_owned(), + action: ErrorAction::IgnoreError}); + } + } + + if msg.channel_flags & 1 == 1 { + check_update_latest(&channel.two_to_one) + } else { + check_update_latest(&channel.one_to_two) + } + }; + let mut channels = self.channels.write().unwrap(); match channels.get_mut(&msg.short_channel_id) { None => { @@ -2264,38 +2305,7 @@ impl NetworkGraph where L::Target: Logger { }); }, Some(channel) => { - if msg.htlc_maximum_msat > MAX_VALUE_MSAT { - return Err(LightningError{err: - "htlc_maximum_msat is larger than maximum possible msats".to_owned(), - action: ErrorAction::IgnoreError}); - } - - if let Some(capacity_sats) = channel.capacity_sats { - // It's possible channel capacity is available now, although it wasn't available at announcement (so the field is None). - // Don't query UTXO set here to reduce DoS risks. - if capacity_sats > MAX_VALUE_MSAT / 1000 || msg.htlc_maximum_msat > capacity_sats * 1000 { - return Err(LightningError{err: - "htlc_maximum_msat is larger than channel capacity or capacity is bogus".to_owned(), - action: ErrorAction::IgnoreError}); - } - } - macro_rules! check_update_latest { - ($target: expr) => { - if let Some(existing_chan_info) = $target.as_ref() { - // The timestamp field is somewhat of a misnomer - the BOLTs use it to - // order updates to ensure you always have the latest one, only - // suggesting that it be at least the current time. For - // channel_updates specifically, the BOLTs discuss the possibility of - // pruning based on the timestamp field being more than two weeks old, - // but only in the non-normative section. - if existing_chan_info.last_update > msg.timestamp { - return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); - } else if existing_chan_info.last_update == msg.timestamp { - return Err(LightningError{err: "Update had same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); - } - } - } - } + check_msg_sanity(channel)?; macro_rules! get_new_channel_info { () => { { @@ -2320,7 +2330,6 @@ impl NetworkGraph where L::Target: Logger { let msg_hash = hash_to_message!(&message_sha256d_hash(&msg)[..]); if msg.channel_flags & 1 == 1 { - check_update_latest!(channel.two_to_one); if let Some(sig) = sig { secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{ err: "Couldn't parse source node pubkey".to_owned(), @@ -2331,7 +2340,6 @@ impl NetworkGraph where L::Target: Logger { channel.two_to_one = get_new_channel_info!(); } } else { - check_update_latest!(channel.one_to_two); if let Some(sig) = sig { secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_one.as_slice()).map_err(|_| LightningError{ err: "Couldn't parse destination node pubkey".to_owned(), From 131849f383336debac86b80a403cc6e64376529a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Sep 2024 23:36:29 +0000 Subject: [PATCH 2/2] Validate `channel_update` signatures without holding a graph lock We often process many gossip messages in parallel across different peer connections, making the `NetworkGraph` mutexes fairly contention-sensitive (not to mention the potential that we want to send a payment and need to find a path to do so). Because we need to look up a node's public key to validate a signature on `channel_update` messages, we always need to take a `NetworkGraph::channels` lock before we can validate the message. For simplicity, and to avoid taking a lock twice, we'd always validated the `channel_update` signature while holding the same lock, but here we address the contention issues by doing a `channel_update` validation in three stages. First we take a read lock on `NetworkGraph::channels` and check if the `channel_update` is new, then release the lock and validate the message signature, and finally take a write lock, (re-check if the `channel_update` is new) and update the graph. --- lightning/src/routing/gossip.rs | 111 ++++++++++++++++---------------- 1 file changed, 57 insertions(+), 54 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 4185ca05ef9..607f8c22eff 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -2294,62 +2294,65 @@ impl NetworkGraph where L::Target: Logger { } }; - let mut channels = self.channels.write().unwrap(); - match channels.get_mut(&msg.short_channel_id) { - None => { - core::mem::drop(channels); - self.pending_checks.check_hold_pending_channel_update(msg, full_msg)?; - return Err(LightningError { - err: "Couldn't find channel for update".to_owned(), - action: ErrorAction::IgnoreAndLog(Level::Gossip), - }); - }, - Some(channel) => { - check_msg_sanity(channel)?; - - macro_rules! get_new_channel_info { - () => { { - let last_update_message = if msg.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY - { full_msg.cloned() } else { None }; - - let updated_channel_update_info = ChannelUpdateInfo { - enabled: chan_enabled, - last_update: msg.timestamp, - cltv_expiry_delta: msg.cltv_expiry_delta, - htlc_minimum_msat: msg.htlc_minimum_msat, - htlc_maximum_msat: msg.htlc_maximum_msat, - fees: RoutingFees { - base_msat: msg.fee_base_msat, - proportional_millionths: msg.fee_proportional_millionths, - }, - last_update_message - }; - Some(updated_channel_update_info) - } } - } - - let msg_hash = hash_to_message!(&message_sha256d_hash(&msg)[..]); - if msg.channel_flags & 1 == 1 { - if let Some(sig) = sig { - secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{ + let node_pubkey; + { + let channels = self.channels.read().unwrap(); + match channels.get(&msg.short_channel_id) { + None => { + core::mem::drop(channels); + self.pending_checks.check_hold_pending_channel_update(msg, full_msg)?; + return Err(LightningError { + err: "Couldn't find channel for update".to_owned(), + action: ErrorAction::IgnoreAndLog(Level::Gossip), + }); + }, + Some(channel) => { + check_msg_sanity(channel)?; + let node_id = if msg.channel_flags & 1 == 1 { + channel.node_two.as_slice() + } 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) - })?, "channel_update"); - } - if !only_verify { - channel.two_to_one = get_new_channel_info!(); - } - } else { - if let Some(sig) = sig { - secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_one.as_slice()).map_err(|_| LightningError{ - err: "Couldn't parse destination node pubkey".to_owned(), - action: ErrorAction::IgnoreAndLog(Level::Debug) - })?, "channel_update"); - } - if !only_verify { - channel.one_to_two = get_new_channel_info!(); - } - } + })?; + }, + } + } + + let msg_hash = hash_to_message!(&message_sha256d_hash(&msg)[..]); + if let Some(sig) = sig { + secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &node_pubkey, "channel_update"); + } + + if only_verify { return Ok(()); } + + let mut channels = self.channels.write().unwrap(); + if let Some(channel) = channels.get_mut(&msg.short_channel_id) { + check_msg_sanity(channel)?; + + let last_update_message = if msg.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY + { full_msg.cloned() } else { None }; + + let new_channel_info = Some(ChannelUpdateInfo { + enabled: chan_enabled, + last_update: msg.timestamp, + cltv_expiry_delta: msg.cltv_expiry_delta, + htlc_minimum_msat: msg.htlc_minimum_msat, + htlc_maximum_msat: msg.htlc_maximum_msat, + fees: RoutingFees { + base_msat: msg.fee_base_msat, + proportional_millionths: msg.fee_proportional_millionths, + }, + last_update_message + }); + + if msg.channel_flags & 1 == 1 { + channel.two_to_one = new_channel_info; + } else { + channel.one_to_two = new_channel_info; } }