From: Matt Corallo <git@bluematt.me>
Date: Wed, 11 Sep 2024 23:36:29 +0000 (+0000)
Subject: Validate `channel_update` signatures without holding a graph lock
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2024-09-unlocked-checksig;p=rust-lightning

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.
---

diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index 4185ca05e..607f8c22e 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -2294,62 +2294,65 @@ impl<L: Deref> NetworkGraph<L> 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;
 			}
 		}