Reintroduce addresses to NodeAnnouncementInfo. #3072

arik-so · 2024-05-21T01:19:47Z

Parsing v2 gossip, which will include socket addresses, but no signatures, will require the ability to store addresses on the NodeAnnouncementInfo object without a corresponding (signed) NodeAnnouncement message.

This approach is to effectively revert commit 6f5e5e3. Some alternative approaches could be setting an enum for either a signed or unsigned node announcement, or even to have an enum for either a signed node announcement or the addresses field.

codecov-commenter · 2024-05-21T01:41:10Z

Codecov Report

Attention: Patch coverage is 95.57522% with 5 lines in your changes missing coverage. Please review.

Project coverage is 91.86%. Comparing base (1d452d0) to head (db489ba).
Report is 81 commits behind head on main.

Files	Patch %	Lines
lightning/src/routing/gossip.rs	95.23%	5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3072      +/-   ##
==========================================
+ Coverage   89.81%   91.86%   +2.04%     
==========================================
  Files         116      119       +3     
  Lines       96467   113993   +17526     
  Branches    96467   113993   +17526     
==========================================
+ Hits        86646   104715   +18069     
+ Misses       7276     6941     -335     
+ Partials     2545     2337     -208

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arik-so · 2024-05-21T22:28:50Z

lightning/src/routing/gossip.rs

+			last_update: last_update.0.unwrap(),
+			rgb: rgb.0.unwrap(),
+			alias: alias.0.unwrap(),
+			addresses: addresses.unwrap_or(Vec::new()),


should this be sourced from the announcement message instead? We are introducing the potential for multiple sources of truth, so feedback would be much appreciated.

Yeah, to avoid this I'd prefer one of the enum approaches that you mention in the PR description.

Honestly, same. Any preference on which one? Also, I think @TheBlueMatt should probably weigh in, too

We'd probably need three variants for:

NodeAnnouncement

NodeAnnouncement that isn't relayed (i.e., when should_relay is false) using a struct containing everything in NodeAnnouncementInfo but announcement_message

UnsignedNodeAnnouncement (maybe with excess_address_data and excess_data cleared)

In that sense, NodeAnnouncementInfo would become an enum, though I'm not sure if there's anything that would make the undesirable. We'd also need accessor methods for the fields.

Alternatively, we could just store an UnsignedNodeAnnouncement, and an optional field for the signatures, whose presence would imply should_relay.

That could work, though we'd need to clear excess_address_data and excess_data when not relaying, too. It would require that we copy the data to produce a NodeAnnouncement during relay, though it looks like the RoutingMessageHandler trait requires us to do that anyway, as currently designed.

The enum approach seems cleaner over some kind of dummy signatures, especially in that we'll get type checking when trying to access the announcement for relay. IMO we should not be storing UnsignedNodeAnnouncements unless we have all the data that goes in them, which in this case we may not, so we should just have one enum variant across unrelayed-announcements and RGS-sourced data.

I think that pretty much means that NodeAnnouncementInfo itself would become a two-variant-enum instead, with the variants being either Relayed(NodeAnnouncement), or Unrelayed(features, last_update, rgb, alias, addresses).

However, given that the unrelayed struct is only missing node_id and the two excess fields from UnsignedNodeAnnouncement, is it worth introducing a separate struct for that? My intuition would be yes, but double-checking anyway.

Yea, I think so? Reusing the same struct implies something specific (that this is the node announcement) whereas a separate struct communicates via the type system that this is just info about the node.

TheBlueMatt

Design LGTM.

TheBlueMatt · 2024-05-29T19:37:07Z

lightning/src/routing/gossip.rs

 	/// Protocol features the node announced support for
-	pub features: NodeFeatures,
+	features: NodeFeatures,


Any reason to not leave these pub?

we have accessors for each of those fields now, so it seemed odd leaving these public

unambiguous usage for developers? But I'll make the fields public

Why is accessing fields ambiguous?

TheBlueMatt · 2024-05-29T19:39:33Z

lightning/src/routing/gossip.rs

+	}
+
+	/// When the last known update to the node state was issued.
+	/// Value is opaque, as set in the announcement.


Suggested change

/// Value is opaque, as set in the announcement.

///

/// Value may or may not be a timestamp, depending on the policy of the origin node.

TheBlueMatt · 2024-05-29T19:40:08Z

lightning/src/routing/gossip.rs

+	/// May be invalid or malicious (eg control chars),
+	/// should not be exposed to the user.


?

Suggested change

/// May be invalid or malicious (eg control chars),

/// should not be exposed to the user.

///

/// May be invalid or malicious (eg control chars), should not be exposed to the user.

lightning/src/routing/gossip.rs

arik-so · 2024-05-30T17:40:18Z

lightning/src/routing/gossip.rs

-					alias: msg.alias,
-					announcement_message: if should_relay { full_msg.cloned() } else { None },
-				});
+				node.announcement_info = if let Some(signed_announcement) = full_msg {


this looks incredibly awkward to me but rust does't allow combining if let with other booleans, so not sure how to improve this

lightning/src/routing/gossip.rs

TheBlueMatt · 2024-05-31T15:16:03Z

lightning/src/routing/gossip.rs

+	}
+
+	/// Moniker assigned to the node.
+	/// May be invalid or malicious (eg control chars), should not be exposed to the user.


Suggested change

/// May be invalid or malicious (eg control chars), should not be exposed to the user.

///

/// May be invalid or malicious (eg control chars), should not be exposed to the user.

TheBlueMatt · 2024-05-31T15:16:15Z

lightning/src/routing/gossip.rs

+	}
+
+	/// An initial announcement of the node
+	/// Not stored if contains excess data to prevent DoS.


Suggested change

/// Not stored if contains excess data to prevent DoS.

///

/// Not stored if contains excess data to prevent DoS.

TheBlueMatt · 2024-05-31T15:18:29Z

lightning/src/routing/gossip.rs

+		let last_update = self.last_update();
+		let rgb = self.rgb();
+		let alias = self.alias();
+		let addresses = self.addresses().to_vec();


If addresses() returned a vec rather than to_slice-ing you could avoid the alloc here, which is going to be pretty brutal when we write 10k nodes.

what do you think is the optimal way of avoiding allocations? Returning a &Vec<SocketAddress> and implementing a new writeable for that type?

You just need a * in the write_tlv_fields call.

thanks! I kept trying to have the right type prior to the macro invocation.

TheBlueMatt · 2024-05-31T15:19:32Z

lightning/src/routing/gossip.rs

@@ -1182,11 +1271,19 @@ impl Readable for NodeAnnouncementInfo {
 			(4, rgb, required),
 			(6, alias, required),
 			(8, announcement_message, option),
-			(10, _addresses, optional_vec), // deprecated, not used anymore
+			(10, addresses, optional_vec),


Did we ever not write this? Can we make it required?

we should be able to

Did you go back and check every release since we made it optional? Also, was it added after the serialization was written or in the original?

I couldn't find any point in history of us not writing it. And yet, it historically has always been an optional_vec read for some reason.

Something odd happened in this commit: a04bf84, where vec_type instances were converted to required_vec, but this one got converted to optional_vec instead

lightning/src/onion_message/messenger.rs

lightning/src/routing/gossip.rs

TheBlueMatt · 2024-06-03T16:19:32Z

LGTM, feel free to squash.

TheBlueMatt

Sorry one more

TheBlueMatt · 2024-06-04T18:06:21Z

lightning/src/onion_message/messenger.rs

-				.map(|node_announcement| &node_announcement.contents);
+				.and_then(|announcement_info| {
+					announcement_info
+						.announcement_message()


This should just use anouncement_info.features().

but the node addresses are also being used later on, and the features alone don't contain them?

Either way the code here doesn't need to rely on us having stored the full announcement, which it currently does.

TheBlueMatt · 2024-06-04T19:16:49Z

lightning/src/onion_message/messenger.rs

-				Some(node_announcement) if node_announcement.features.supports_onion_messages() => {
-					let first_node_addresses = Some(node_announcement.addresses.clone());
+			match node_details {
+				Some((features, addresses)) if features.supports_onion_messages() => {


Oh whoops we should also filter by if we have an address or not.

TheBlueMatt · 2024-06-04T21:26:57Z

lightning/src/routing/gossip.rs

+			(4, rgb, required),
+			(6, alias, required),
+			(8, announcement_message, option),
+			(10, *addresses, required_vec), // Versions prior to 0.0.115 require this field


This comment can be removed - we now require the field today too.

any preferences between Versions between 0.0.115 and 0.0.123 did not require this field and leaving it out altogether?

I don't have a strong feeling. If we leave a comment it should probably note that those versions wrote empty Vecs, which imo is more relevant than the fact that they would accept no Vec (which we can't take advantage of in the future as we are now going to require it again).

arik-so force-pushed the arik/gossip-v2-parsing branch from 5a03472 to 5d9d796 Compare May 21, 2024 01:31

arik-so force-pushed the arik/gossip-v2-parsing branch from 5d9d796 to cb08eb2 Compare May 21, 2024 01:42

arik-so marked this pull request as ready for review May 21, 2024 09:22

arik-so force-pushed the arik/gossip-v2-parsing branch from cb08eb2 to 4bf6a8c Compare May 21, 2024 22:26

arik-so commented May 21, 2024

View reviewed changes

jkczyz self-requested a review May 21, 2024 22:31

arik-so force-pushed the arik/gossip-v2-parsing branch from a0efc92 to a8b2aa9 Compare May 29, 2024 03:02

TheBlueMatt reviewed May 29, 2024

View reviewed changes

arik-so force-pushed the arik/gossip-v2-parsing branch from a8b2aa9 to baafcdf Compare May 29, 2024 21:48

arik-so commented May 30, 2024

View reviewed changes

TheBlueMatt reviewed May 31, 2024

View reviewed changes

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved

arik-so force-pushed the arik/gossip-v2-parsing branch from f8df9fc to 9770247 Compare May 31, 2024 16:32

TheBlueMatt reviewed May 31, 2024

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

arik-so force-pushed the arik/gossip-v2-parsing branch from 29df13c to e9a0c91 Compare June 1, 2024 05:11

arik-so force-pushed the arik/gossip-v2-parsing branch from e9a0c91 to 3ee0a00 Compare June 3, 2024 17:16

TheBlueMatt reviewed Jun 4, 2024

View reviewed changes

arik-so force-pushed the arik/gossip-v2-parsing branch from 3ee0a00 to 06f9510 Compare June 4, 2024 18:19

TheBlueMatt reviewed Jun 4, 2024

View reviewed changes

arik-so force-pushed the arik/gossip-v2-parsing branch from 06f9510 to bbc291f Compare June 4, 2024 19:25

TheBlueMatt previously approved these changes Jun 4, 2024

View reviewed changes

Reintroduce addresses to NodeAnnouncementInfo.

db489ba

arik-so dismissed TheBlueMatt’s stale review via db489ba June 5, 2024 16:49

arik-so force-pushed the arik/gossip-v2-parsing branch from bbc291f to db489ba Compare June 5, 2024 16:49

TheBlueMatt approved these changes Jun 5, 2024

View reviewed changes

jkczyz approved these changes Jun 5, 2024

View reviewed changes

TheBlueMatt merged commit 2701bc5 into lightningdevkit:main Jun 5, 2024
16 checks passed

	/// Value is opaque, as set in the announcement.
	///
	/// Value may or may not be a timestamp, depending on the policy of the origin node.

		/// May be invalid or malicious (eg control chars),
		/// should not be exposed to the user.

	/// Not stored if contains excess data to prevent DoS.
	///
	/// Not stored if contains excess data to prevent DoS.

Reintroduce addresses to NodeAnnouncementInfo. #3072

Reintroduce addresses to NodeAnnouncementInfo. #3072

Conversation

arik-so commented May 21, 2024 • edited Loading

codecov-commenter commented May 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 3, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so commented May 21, 2024 •

edited

Loading

codecov-commenter commented May 21, 2024 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading