From: Matt Corallo <git@bluematt.me>
Date: Sun, 21 Aug 2022 20:34:22 +0000 (+0000)
Subject: Avoid querying the chain for outputs for channels we already have
X-Git-Tag: v0.0.111~28^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=5847abd92db2f5346bdc307ec532df6a3753dec3;p=rust-lightning

Avoid querying the chain for outputs for channels we already have

If we receive a ChannelAnnouncement message but we already have the
channel, there's no reason to do a chain lookup. Instead of
immediately calling the user-provided `chain::Access` when handling
a ChannelAnnouncement, we first check if we have the corresponding
channel in the graph.

Note that if we do have the corresponding channel but it was not
previously checked against the blockchain, we should still check
with the `chain::Access` and update if necessary.
---

diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index b083f402..16060921 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -1433,6 +1433,39 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 			return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError});
 		}
 
+		{
+			let channels = self.channels.read().unwrap();
+
+			if let Some(chan) = channels.get(&msg.short_channel_id) {
+				if chan.capacity_sats.is_some() {
+					// If we'd previously looked up the channel on-chain and checked the script
+					// against what appears on-chain, ignore the duplicate announcement.
+					//
+					// Because a reorg could replace one channel with another at the same SCID, if
+					// the channel appears to be different, we re-validate. This doesn't expose us
+					// to any more DoS risk than not, as a peer can always flood us with
+					// randomly-generated SCID values anyway.
+					//
+					// We use the Node IDs rather than the bitcoin_keys to check for "equivalence"
+					// as we didn't (necessarily) store the bitcoin keys, and we only really care
+					// if the peers on the channel changed anyway.
+					if NodeId::from_pubkey(&msg.node_id_1) == chan.node_one && NodeId::from_pubkey(&msg.node_id_2) == chan.node_two {
+						return Err(LightningError {
+							err: "Already have chain-validated channel".to_owned(),
+							action: ErrorAction::IgnoreDuplicateGossip
+						});
+					}
+				} else if chain_access.is_none() {
+					// Similarly, if we can't check the chain right now anyway, ignore the
+					// duplicate announcement without bothering to take the channels write lock.
+					return Err(LightningError {
+						err: "Already have non-chain-validated channel".to_owned(),
+						action: ErrorAction::IgnoreDuplicateGossip
+					});
+				}
+			}
+		}
+
 		let utxo_value = match &chain_access {
 			&None => {
 				// Tentatively accept, potentially exposing us to DoS attacks
@@ -2046,7 +2079,7 @@ mod tests {
 		// drop new one on the floor, since we can't see any changes.
 		match gossip_sync.handle_channel_announcement(&valid_announcement) {
 			Ok(_) => panic!(),
-			Err(e) => assert_eq!(e.err, "Already have knowledge of channel")
+			Err(e) => assert_eq!(e.err, "Already have non-chain-validated channel")
 		};
 
 		// Test if an associated transaction were not on-chain (or not confirmed).
@@ -2080,32 +2113,13 @@ mod tests {
 			};
 		}
 
-		// If we receive announcement for the same channel (but TX is not confirmed),
-		// drop new one on the floor, since we can't see any changes.
-		*chain_source.utxo_ret.lock().unwrap() = Err(chain::AccessError::UnknownTx);
-		match gossip_sync.handle_channel_announcement(&valid_announcement) {
-			Ok(_) => panic!(),
-			Err(e) => assert_eq!(e.err, "Channel announced without corresponding UTXO entry")
-		};
-
-		// But if it is confirmed, replace the channel
+		// If we receive announcement for the same channel, once we've validated it against the
+		// chain, we simply ignore all new (duplicate) announcements.
 		*chain_source.utxo_ret.lock().unwrap() = Ok(TxOut { value: 0, script_pubkey: good_script });
-		let valid_announcement = get_signed_channel_announcement(|unsigned_announcement| {
-			unsigned_announcement.features = ChannelFeatures::empty();
-			unsigned_announcement.short_channel_id += 2;
-		}, node_1_privkey, node_2_privkey, &secp_ctx);
 		match gossip_sync.handle_channel_announcement(&valid_announcement) {
-			Ok(res) => assert!(res),
-			_ => panic!()
+			Ok(_) => panic!(),
+			Err(e) => assert_eq!(e.err, "Already have chain-validated channel")
 		};
-		{
-			match network_graph.read_only().channels().get(&valid_announcement.contents.short_channel_id) {
-				Some(channel_entry) => {
-					assert_eq!(channel_entry.features, ChannelFeatures::empty());
-				},
-				_ => panic!()
-			};
-		}
 
 		// Don't relay valid channels with excess data
 		let valid_announcement = get_signed_channel_announcement(|unsigned_announcement| {