Merge pull request #1679 from TheBlueMatt/2022-08-no-double-access

Avoid querying the chain for outputs for channels we already have

diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index b083f402141f9566d1b57afbc4dd75b4937a8009..160609216e84bc2c36ee0f1d2f6388fff657d9ac 100644 (file)
--- 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| {