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.

Author: douglaz

lightning/src/routing/gossip.rs

@@ -1046,23 +1046,52 @@ 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.
 	/// Not stored if contains excess data to prevent DoS.
 	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"))]