From 6f5e5e3898da67da809e939e6980bc3fbe647112 Mon Sep 17 00:00:00 2001 From: "Allan Douglas R. de Oliveira" Date: Mon, 13 Mar 2023 20:13:28 +0000 Subject: [PATCH] Remove redundant `addresses` field from `NodeAnnouncementInfo` ...replacing it with an acessor `addresses()`. Besides removing a redundant data structure already present on inner `NodeAnnouncement`, this change makes it possible to discover new address types upon deserialization thanks to `UnsignedNodeAnnouncement`'s implementation. --- lightning/src/routing/gossip.rs | 88 ++++++++++++++++++++++----------- 1 file changed, 60 insertions(+), 28 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 3f9313c68..ae2378c38 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -1046,8 +1046,6 @@ pub struct NodeAnnouncementInfo { /// May be invalid or malicious (eg control chars), /// should not be exposed to the user. pub alias: NodeAlias, - /// Internet-level addresses via which one can connect to the node - pub addresses: Vec, /// An initial announcement of the node /// Mostly redundant with the data we store in fields explicitly. /// Everything else is useful only for sending out for initial routing sync. @@ -1055,14 +1053,45 @@ pub struct NodeAnnouncementInfo { pub announcement_message: Option } -impl_writeable_tlv_based!(NodeAnnouncementInfo, { - (0, features, required), - (2, last_update, required), - (4, rgb, required), - (6, alias, required), - (8, announcement_message, option), - (10, addresses, vec_type), -}); +impl NodeAnnouncementInfo { + /// Internet-level addresses via which one can connect to the node + pub fn addresses(&self) -> &[NetAddress] { + self.announcement_message.as_ref() + .map(|msg| msg.contents.addresses.as_slice()) + .unwrap_or_default() + } +} + +impl Writeable for NodeAnnouncementInfo { + fn write(&self, writer: &mut W) -> Result<(), io::Error> { + let empty_addresses = Vec::::new(); + write_tlv_fields!(writer, { + (0, self.features, required), + (2, self.last_update, required), + (4, self.rgb, required), + (6, self.alias, required), + (8, self.announcement_message, option), + (10, empty_addresses, vec_type), // Versions prior to 0.0.115 require this field + }); + Ok(()) + } +} + +impl Readable for NodeAnnouncementInfo { + fn read(reader: &mut R) -> Result { + _init_and_read_tlv_fields!(reader, { + (0, features, required), + (2, last_update, required), + (4, rgb, required), + (6, alias, required), + (8, announcement_message, option), + (10, _addresses, vec_type), // deprecated, not used anymore + }); + let _: Option> = _addresses; + Ok(Self { features: features.0.unwrap(), last_update: last_update.0.unwrap(), rgb: rgb.0.unwrap(), + alias: alias.0.unwrap(), announcement_message }) + } +} /// A user-defined name for a node, which may be used when displaying the node in a graph. /// @@ -1133,7 +1162,7 @@ impl Writeable for NodeInfo { } } -// A wrapper allowing for the optional deseralization of `NodeAnnouncementInfo`. Utilizing this is +// A wrapper allowing for the optional deserialization of `NodeAnnouncementInfo`. Utilizing this is // necessary to maintain compatibility with previous serializations of `NetAddress` that have an // invalid hostname set. We ignore and eat all errors until we are either able to read a // `NodeAnnouncementInfo` or hit a `ShortRead`, i.e., read the TLV field to the end. @@ -1362,7 +1391,6 @@ impl NetworkGraph where L::Target: Logger { last_update: msg.timestamp, rgb: msg.rgb, alias: NodeAlias(msg.alias), - addresses: msg.addresses.clone(), announcement_message: if should_relay { full_msg.cloned() } else { None }, }); @@ -1916,12 +1944,8 @@ impl ReadOnlyNetworkGraph<'_> { /// Returns None if the requested node is completely unknown, /// or if node announcement for the node was never received. pub fn get_addresses(&self, pubkey: &PublicKey) -> Option> { - if let Some(node) = self.nodes.get(&NodeId::from_pubkey(&pubkey)) { - if let Some(node_info) = node.announcement_info.as_ref() { - return Some(node_info.addresses.clone()) - } - } - None + self.nodes.get(&NodeId::from_pubkey(&pubkey)) + .and_then(|node| node.announcement_info.as_ref().map(|ann| ann.addresses().to_vec())) } } @@ -1938,7 +1962,7 @@ pub(crate) mod tests { ReplyChannelRange, QueryChannelRange, QueryShortChannelIds, MAX_VALUE_MSAT}; use crate::util::config::UserConfig; use crate::util::test_utils; - use crate::util::ser::{ReadableArgs, Writeable}; + use crate::util::ser::{ReadableArgs, Readable, Writeable}; use crate::util::events::{MessageSendEvent, MessageSendEventsProvider}; use crate::util::scid_utils::scid_from_parts; @@ -3244,26 +3268,25 @@ pub(crate) mod tests { #[test] fn node_info_is_readable() { - use std::convert::TryFrom; - // 1. Check we can read a valid NodeAnnouncementInfo and fail on an invalid one - let valid_netaddr = crate::ln::msgs::NetAddress::Hostname { hostname: crate::util::ser::Hostname::try_from("A".to_string()).unwrap(), port: 1234 }; + let announcement_message = hex::decode("d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a000122013413a7031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f2020201010101010101010101010101010101010101010101010101010101010101010000701fffefdfc2607").unwrap(); + let announcement_message = NodeAnnouncement::read(&mut announcement_message.as_slice()).unwrap(); let valid_node_ann_info = NodeAnnouncementInfo { features: channelmanager::provided_node_features(&UserConfig::default()), last_update: 0, rgb: [0u8; 3], alias: NodeAlias([0u8; 32]), - addresses: vec![valid_netaddr], - announcement_message: None, + announcement_message: Some(announcement_message) }; let mut encoded_valid_node_ann_info = Vec::new(); assert!(valid_node_ann_info.write(&mut encoded_valid_node_ann_info).is_ok()); - let read_valid_node_ann_info: NodeAnnouncementInfo = crate::util::ser::Readable::read(&mut encoded_valid_node_ann_info.as_slice()).unwrap(); + let read_valid_node_ann_info = NodeAnnouncementInfo::read(&mut encoded_valid_node_ann_info.as_slice()).unwrap(); assert_eq!(read_valid_node_ann_info, valid_node_ann_info); + assert_eq!(read_valid_node_ann_info.addresses().len(), 1); let encoded_invalid_node_ann_info = hex::decode("3f0009000788a000080a51a20204000000000403000000062000000000000000000000000000000000000000000000000000000000000000000a0505014004d2").unwrap(); - let read_invalid_node_ann_info_res: Result = crate::util::ser::Readable::read(&mut encoded_invalid_node_ann_info.as_slice()); + let read_invalid_node_ann_info_res = NodeAnnouncementInfo::read(&mut encoded_invalid_node_ann_info.as_slice()); assert!(read_invalid_node_ann_info_res.is_err()); // 2. Check we can read a NodeInfo anyways, but set the NodeAnnouncementInfo to None if invalid @@ -3274,13 +3297,22 @@ pub(crate) mod tests { let mut encoded_valid_node_info = Vec::new(); assert!(valid_node_info.write(&mut encoded_valid_node_info).is_ok()); - let read_valid_node_info: NodeInfo = crate::util::ser::Readable::read(&mut encoded_valid_node_info.as_slice()).unwrap(); + let read_valid_node_info = NodeInfo::read(&mut encoded_valid_node_info.as_slice()).unwrap(); assert_eq!(read_valid_node_info, valid_node_info); let encoded_invalid_node_info_hex = hex::decode("4402403f0009000788a000080a51a20204000000000403000000062000000000000000000000000000000000000000000000000000000000000000000a0505014004d20400").unwrap(); - let read_invalid_node_info: NodeInfo = crate::util::ser::Readable::read(&mut encoded_invalid_node_info_hex.as_slice()).unwrap(); + let read_invalid_node_info = NodeInfo::read(&mut encoded_invalid_node_info_hex.as_slice()).unwrap(); assert_eq!(read_invalid_node_info.announcement_info, None); } + + #[test] + fn test_node_info_keeps_compatibility() { + let old_ann_info_with_addresses = hex::decode("3f0009000708a000080a51220204000000000403000000062000000000000000000000000000000000000000000000000000000000000000000a0505014104d2").unwrap(); + let ann_info_with_addresses = NodeAnnouncementInfo::read(&mut old_ann_info_with_addresses.as_slice()) + .expect("to be able to read an old NodeAnnouncementInfo with addresses"); + // This serialized info has an address field but no announcement_message, therefore the addresses returned by our function will still be empty + assert!(ann_info_with_addresses.addresses().is_empty()); + } } #[cfg(all(test, feature = "_bench_unstable"))] -- 2.39.5