From 56cb6a10f1437c5da593b1e093a55bd095dd7aaa Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 8 Sep 2024 22:33:57 +0000 Subject: [PATCH] Avoid redundant `{channel,node}_announcement` signature checks 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. --- lightning/src/routing/gossip.rs | 115 +++++++++++++++++++------------- 1 file changed, 70 insertions(+), 45 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index f1bc73110..a1060a7ac 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -1720,6 +1720,15 @@ impl NetworkGraph where L::Target: Logger { /// RoutingMessageHandler implementation to call it indirectly. This may be useful to accept /// routing messages from a source using a protocol other than the lightning P2P protocol. 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) = 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.contents.timestamp { + return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); + } + } + } verify_node_announcement(msg, &self.secp_ctx)?; self.update_node_from_announcement_intern(&msg.contents, Some(&msg)) } @@ -1788,6 +1797,7 @@ impl NetworkGraph where L::Target: Logger { where U::Target: UtxoLookup, { + self.pre_channel_announcement_validation_check(&msg.contents, utxo_lookup)?; verify_channel_announcement(msg, &self.secp_ctx)?; self.update_channel_from_unsigned_announcement_intern(&msg.contents, Some(msg), utxo_lookup) } @@ -1817,6 +1827,7 @@ impl NetworkGraph where L::Target: Logger { where U::Target: UtxoLookup, { + self.pre_channel_announcement_validation_check(&msg, utxo_lookup)?; self.update_channel_from_unsigned_announcement_intern(msg, None, utxo_lookup) } @@ -1911,6 +1922,52 @@ impl NetworkGraph where L::Target: Logger { Ok(()) } + /// If we already have all the information for a channel that we're gonna get, there's no + /// reason to redundantly process it. + /// + /// In those cases, this will return an `Err` that we can return immediately. Otherwise it will + /// return an `Ok(())`. + fn pre_channel_announcement_validation_check( + &self, msg: &msgs::UnsignedChannelAnnouncement, utxo_lookup: &Option, + ) -> Result<(), LightningError> where U::Target: UtxoLookup { + 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 msg.node_id_1 == chan.node_one && msg.node_id_2 == chan.node_two { + return Err(LightningError { + err: "Already have chain-validated channel".to_owned(), + action: ErrorAction::IgnoreDuplicateGossip + }); + } + } else if utxo_lookup.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 + }); + } + } + + Ok(()) + } + + /// Update channel information from a received announcement. + /// + /// Generally [`Self::pre_channel_announcement_validation_check`] should have been called + /// first. fn update_channel_from_unsigned_announcement_intern( &self, msg: &msgs::UnsignedChannelAnnouncement, full_msg: Option<&msgs::ChannelAnnouncement>, utxo_lookup: &Option ) -> Result<(), LightningError> @@ -1928,39 +1985,6 @@ impl NetworkGraph where L::Target: Logger { }); } - { - 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 msg.node_id_1 == chan.node_one && msg.node_id_2 == chan.node_two { - return Err(LightningError { - err: "Already have chain-validated channel".to_owned(), - action: ErrorAction::IgnoreDuplicateGossip - }); - } - } else if utxo_lookup.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 removed_channels = self.removed_channels.lock().unwrap(); let removed_nodes = self.removed_nodes.lock().unwrap(); @@ -2562,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 { @@ -2577,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); @@ -2698,23 +2722,24 @@ 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!(), -- 2.39.5