Avoid redundant {channel,node}_announcement signature checks

[TheBlueMatt]

If we receive {channel,node}_announcement messages which we already have, we first validate their signatures and then look in our graph and discover that we should discard the messages. This avoids a second lock in node_announcement handling but does not impact our locking in channel_announcement handling. It also avoids lock contention in cases where the signatures are invalid, but that should be exceedingly rare.

For nodes with relatively few peers, this is a fine state to be in, however for nodes with many peers, we may see the same messages hundreds of times. This causes a rather substantial waste of CPU resources validating gossip messages.

Instead, here, we change to checking our network graph first and then validate the signatures only if we don't already have the message.

[tnull]

Unfortunately this doesn't build.

This makes cargo build happy, but two test cases are still failing for me:

diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index e5a2f1d08..cea44eaa8 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -1722,9 +1722,9 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
        pub fn update_node_from_announcement(&self, msg: &msgs::NodeAnnouncement) -> Result<(), LightningError> {
                // First check if we have the announcement already to avoid the CPU cost of validating a
                // redundant announcement.
-               if let Some(node) = nodes.self.nodes.read().unwrap().get(&msg.node_id) {
+               if let Some(node) = self.nodes.read().unwrap().get(&msg.contents.node_id) {
                        if let Some(node_info) = node.announcement_info.as_ref() {
-                               if node_info.last_update()  == msg.timestamp {
+                               if node_info.last_update()  == msg.contents.timestamp {
                                        return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
                                }
                        }
@@ -1827,7 +1827,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
        where
                U::Target: UtxoLookup,
        {
-               self.pre_channel_announcement_validation_check(&msg.contents, utxo_lookup)?;
+               self.pre_channel_announcement_validation_check(&msg, utxo_lookup)?;
                self.update_channel_from_unsigned_announcement_intern(msg, None, utxo_lookup)
        }

@@ -1960,6 +1960,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
                                });
                        }
                }
+               Ok(())
        }

        /// Update channel information from a received announcement.

[TheBlueMatt]

🤦 juggling too many branches at once guess I forgot to test again after fixing other issues.

$ git diff-tree -U3 a19a01e45 75a9c2821
diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index e5a2f1d08..4c11f4ebf 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -1722,9 +1722,9 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 	pub fn update_node_from_announcement(&self, msg: &msgs::NodeAnnouncement) -> Result<(), LightningError> {
 		// First check if we have the announcement already to avoid the CPU cost of validating a
 		// redundant announcement.
-		if let Some(node) = nodes.self.nodes.read().unwrap().get(&msg.node_id) {
+		if let Some(node) = self.nodes.read().unwrap().get(&msg.contents.node_id) {
 			if let Some(node_info) = node.announcement_info.as_ref() {
-				if node_info.last_update()  == msg.timestamp {
+				if node_info.last_update()  == msg.contents.timestamp {
 					return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
 				}
 			}
@@ -1827,7 +1827,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 	where
 		U::Target: UtxoLookup,
 	{
-		self.pre_channel_announcement_validation_check(&msg.contents, utxo_lookup)?;
+		self.pre_channel_announcement_validation_check(&msg, utxo_lookup)?;
 		self.update_channel_from_unsigned_announcement_intern(msg, None, utxo_lookup)
 	}

@@ -1960,6 +1960,8 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 				});
 			}
 		}
+
+		Ok(())
 	}

 	/// Update channel information from a received announcement.
@@ -2584,11 +2586,6 @@ pub(crate) mod tests {
 			};
 		}

-		match gossip_sync.handle_node_announcement(&valid_announcement) {
-			Ok(res) => assert!(res),
-			Err(_) => panic!()
-		};
-
 		let fake_msghash = hash_to_message!(zero_hash.as_byte_array());
 		match gossip_sync.handle_node_announcement(
 			&NodeAnnouncement {
@@ -2599,6 +2596,11 @@ pub(crate) mod tests {
 			Err(e) => assert_eq!(e.err, "Invalid signature on node_announcement message")
 		};

+		match gossip_sync.handle_node_announcement(&valid_announcement) {
+			Ok(res) => assert!(res),
+			Err(_) => panic!()
+		};
+
 		let announcement_with_data = get_signed_node_announcement(|unsigned_announcement| {
 			unsigned_announcement.timestamp += 1000;
 			unsigned_announcement.excess_data.resize(MAX_EXCESS_BYTES_FOR_RELAY + 1, 0);
@@ -2720,23 +2722,25 @@ pub(crate) mod tests {
 			}
 		}

-		// Don't relay valid channels with excess data
-		let valid_announcement = get_signed_channel_announcement(|unsigned_announcement| {
+		let valid_excess_data_announcement = get_signed_channel_announcement(|unsigned_announcement| {
 			unsigned_announcement.short_channel_id += 4;
 			unsigned_announcement.excess_data.resize(MAX_EXCESS_BYTES_FOR_RELAY + 1, 0);
 		}, node_1_privkey, node_2_privkey, &secp_ctx);
-		match gossip_sync.handle_channel_announcement(&valid_announcement) {
-			Ok(res) => assert!(!res),
-			_ => panic!()
-		};

-		let mut invalid_sig_announcement = valid_announcement.clone();
+		let mut invalid_sig_announcement = valid_excess_data_announcement.clone();
 		invalid_sig_announcement.contents.excess_data = Vec::new();
 		match gossip_sync.handle_channel_announcement(&invalid_sig_announcement) {
 			Ok(_) => panic!(),
 			Err(e) => assert_eq!(e.err, "Invalid signature on channel_announcement message")
 		};

+		// Don't relay valid channels with excess data
+
+		match gossip_sync.handle_channel_announcement(&valid_excess_data_announcement) {
+			Ok(res) => assert!(!res),
+			_ => panic!()
+		};
+
 		let channel_to_itself_announcement = get_signed_channel_announcement(|_| {}, node_1_privkey, node_1_privkey, &secp_ctx);
 		match gossip_sync.handle_channel_announcement(&channel_to_itself_announcement) {
 			Ok(_) => panic!(),
$

[codecov]

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (d35239c) to head (56cb6a1).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/gossip.rs	84.21%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3305      +/-   ##
==========================================
- Coverage   89.85%   89.62%   -0.23%     
==========================================
  Files         126      126              
  Lines      104145   102181    -1964     
  Branches   104145   102181    -1964     
==========================================
- Hits        93577    91582    -1995     
+ Misses       7894     7877      -17     
- Partials     2674     2722      +48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

LGTM

[vincenzopalazzo]

LGTM

[valentinewallace]

Looks like the CI failures are unrelated so landing this