Remove redundant `addresses` field from `NodeAnnouncementInfo`
authorAllan Douglas R. de Oliveira <allandouglas@gmail.com>
Mon, 13 Mar 2023 20:13:28 +0000 (20:13 +0000)
committerAllan Douglas R. de Oliveira <allandouglas@gmail.com>
Tue, 21 Mar 2023 17:27:48 +0000 (17:27 +0000)
...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

index 3f9313c686aab4bd1454f8b8bda84f762afee36a..ae2378c38242b82fc467f1bc9322ca588ec75cc4 100644 (file)
@@ -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<NetAddress>,
        /// 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<NodeAnnouncement>
 }
 
-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<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+               let empty_addresses = Vec::<NetAddress>::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<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+               _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<Vec<NetAddress>> = _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<L: Deref> NetworkGraph<L> 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<Vec<NetAddress>> {
-               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<NodeAnnouncementInfo, crate::ln::msgs::DecodeError> = 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"))]