Move broadcast_node_announcement to PeerManager #1699

TheBlueMatt · 2022-09-06T22:39:05Z

Some NodeFeatures will, in the future, represent features which
are not enabled by the ChannelManager, but by other message
handlers handlers. Thus, it doesn't make sense to determine the
node feature bits in the ChannelManager.

The simplest fix for this is to change to generating the
node_announcement in PeerManager, asking all the connected
handlers which feature bits they support and simply OR'ing them
together. While this may not be sufficient in the future as it
doesn't consider feature bit dependencies, support for those could
be handled at the feature level in the future.

This PR moves channel_announcement/update messages into peer connection handling and moves the broadcast_node_announcement function to
PeerHandler but does not yet implement feature OR'ing.

lightning/src/ln/channelmanager.rs

lightning/src/ln/msgs.rs

codecov-commenter · 2022-09-06T23:46:15Z

Codecov Report

Merging #1699 (25a10a1) into main (301efc8) will decrease coverage by 0.13%.
The diff coverage is 74.28%.

❗ Current head 25a10a1 differs from pull request most recent head e2495f2. Consider uploading reports for the commit e2495f2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1699      +/-   ##
==========================================
- Coverage   90.92%   90.78%   -0.14%     
==========================================
  Files          86       86              
  Lines       46417    46407      -10     
  Branches    46417    46407      -10     
==========================================
- Hits        42204    42132      -72     
- Misses       4213     4275      +62

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`93.68% <ø> (+0.09%)`	⬆️
lightning/src/ln/msgs.rs	`86.24% <ø> (ø)`
lightning/src/util/events.rs	`36.58% <0.00%> (-2.92%)`	⬇️
lightning/src/ln/peer_handler.rs	`57.00% <30.00%> (-0.02%)`	⬇️
lightning/src/util/test_utils.rs	`77.18% <33.33%> (-0.31%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.82% <75.00%> (-0.55%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.94% <84.61%> (-0.06%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.90% <91.66%> (-0.21%)`	⬇️
lightning-background-processor/src/lib.rs	`96.23% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.72% <100.00%> (ø)`
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TheBlueMatt · 2022-09-07T16:48:34Z

Rebased to fix a new test added in git.

lightning/src/ln/functional_test_utils.rs

pending_changelog/1699.txt

jkczyz · 2022-09-07T20:08:08Z

lightning/src/ln/peer_handler.rs

+	/// `current_time` should be set higher than any previous `PeerManager` instance with the same
+	/// secret key. The simplest way to ensure this, of course, is to simply set it to the current
+	/// UNIX timestamp on startup.


This isn't quite accurate or at least is unclear what is meant by "any previous PeerManager instance". It needs to be set higher than what was used to previously initialize plus the number of times that broadcast_node_announcement had previously been called. Maybe just say it should set to the current timestamp?

Tried to reword it. I don't want to be so strict as to tell users they need a clock here.

CONTRIBUTING.md

jkczyz · 2022-09-07T20:17:32Z

pending_changelog/1699.txt

@@ -0,0 +1,2 @@
+`broadcast_node_announcement` has been moved to `PeerManager` from `ChannelManager`.
+`PeerManager::new`'s new `current_time` argument must be set to the current time, not a counter, for any existing nodes which upgrade.


Why bother saying "not a counter"? The counter was previously an implementation detail.

Also, shouldn't all nodes need to set it to the current time regardless if upgrading?

I guess the intent is that users can totally still use a counter if they want, but I'll drop the clause.

Re-added it with a bit more context now that there's similarly more context on the PeerManager constructors themselves.

valentinewallace · 2022-09-07T22:01:29Z

lightning/src/ln/functional_test_utils.rs

-		}
-	}
-	assert!(found_ann_1);
+	assert_eq!(a_events.len(), 1);


Why not still check for the node announcement?

We don't do a node announcement anymore that can be checked here - we don't have a PeerManager in the normal functional test environment, so can't test it.

lightning/src/ln/channelmanager.rs

lightning/src/util/events.rs

lightning/src/ln/peer_handler.rs

valentinewallace · 2022-09-08T15:40:16Z

lightning/src/ln/peer_handler.rs

+			excess_data: Vec::new(),
+		};
+		let msghash = hash_to_message!(&Sha256dHash::hash(&announcement.encode()[..])[..]);
+		let node_announce_sig = sign(&self.secp_ctx, &msghash, &self.our_node_secret);


Hmm, so since PeerManager doesn't have a KeysInterface, it's not obvious to me how we'll get rid of this in the future (unlike with ChannelManager)?

We need to move to PeerManager having a KeysInterface :). We need that anyway as we need to replace the peer ECDH with the KeysInterface ECDH.

lightning/src/ln/peer_handler.rs

lightning/src/util/events.rs

valentinewallace · 2022-09-08T16:28:29Z

pending_changelog/1699.txt

@@ -0,0 +1,2 @@
+`broadcast_node_announcement` has been moved to `PeerManager` from `ChannelManager`.


nit: Maybe we should use the commit hash instead of PR number, to keep the commit history independent from GH?

I dont think the file name matters at all. It just has to be unique, for all that it matters people could just put their name as long as things are unique.

lightning/src/ln/peer_handler.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-09-08T16:51:25Z

lightning/src/ln/peer_handler.rs

 	/// ephemeral_random_data is used to derive per-connection ephemeral keys and must be
 	/// cryptographically secure random bytes.
 	///
 	/// (C-not exported) as we can't export a PeerManager with a dummy channel handler
-	pub fn new_routing_only(routing_message_handler: RM, our_node_secret: SecretKey, ephemeral_random_data: &[u8; 32], logger: L) -> Self {
+	pub fn new_routing_only(routing_message_handler: RM, our_node_secret: SecretKey, current_time: u64, ephemeral_random_data: &[u8; 32], logger: L) -> Self {


Probably doesn't work, but a thought: rather than the user supplying the current time and awkward docs, why not do what ChannelManager did previously and set it to 0 on startup, then compare_exchange it based on ChannelUpdates, etc that we receive that contain timestamps?

We can't be confident in the timestamps we get from random peers. ChannelManager used actual blocks, which we dont currently have in the peer handling, and I'm not sure its worth adding just for this.

valentinewallace

Basically LGTM if CI's happy

lightning/src/ln/channelmanager.rs

valentinewallace · 2022-09-08T18:49:03Z

Looks good, fine to squash whenever on my end

CONTRIBUTING.md

jkczyz · 2022-09-08T18:53:25Z

lightning/src/ln/peer_handler.rs

@@ -200,6 +202,7 @@ impl ChannelMessageHandler for ErroringMessageHandler {
 	fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {}
 	fn peer_connected(&self, _their_node_id: &PublicKey, _msg: &msgs::Init) {}
 	fn handle_error(&self, _their_node_id: &PublicKey, _msg: &msgs::ErrorMessage) {}
+	fn provided_node_features(&self) -> NodeFeatures { NodeFeatures::empty() }


Should this be known? Think it might be changed in the follow-up already.

I think node features can be none, it just means no one will connect to us, which is good. We set more features in init, because we want to make connections to people, but I think node should be none.

jkczyz · 2022-09-08T19:09:46Z

lightning/src/ln/peer_handler.rs

+	/// `current_time` is used as an always-increasing counter that survives across restarts and is
+	/// incremented irregularly internally. In general it is best to simply use the current UNIX
+	/// timestamp, however if it is not available a persistent counter that increases once per
+	/// minute should suffice.
+	///
 	/// (C-not exported) as we can't export a PeerManager with a dummy route handler
-	pub fn new_channel_only(channel_message_handler: CM, onion_message_handler: OM, our_node_secret: SecretKey, ephemeral_random_data: &[u8; 32], logger: L) -> Self {
+	pub fn new_channel_only(channel_message_handler: CM, onion_message_handler: OM, our_node_secret: SecretKey, current_time: u64, ephemeral_random_data: &[u8; 32], logger: L) -> Self {


FWIW, we could always parameterize PeerManager by util::time::Time and provide implementations for (1) SystemTime and (2) a global counter. The latter would be for no-std and could be persisted when incremented. Would probably need an associated function to initialize the counter from persisted data / specify where to persist.

Since neither implementation needs to be instantiated (util::time::Time only has associated functions), we wouldn't need an extra method parameter anywhere. Just an idea. Happy to leave it to a follow-up, if desired.

The issue is the global counter would have to have some kind of persistence, which would mean we need the user involved. Otherwise we'd have a different API between no-std and std which I'm also not a fan of :/

When we connect to a new peer, immediately send them any channel_announcement and channel_update messages for any public channels we have with other peers. This allows us to stop sending those messages on a timer when they have not changed and ensures we are sending messages when we have peers connected, rather than broadcasting at startup when we have no peers connected.

Some `NodeFeatures` will, in the future, represent features which are not enabled by the `ChannelManager`, but by other message handlers handlers. Thus, it doesn't make sense to determine the node feature bits in the `ChannelManager`. The simplest fix for this is to change to generating the node_announcement in `PeerManager`, asking all the connected handlers which feature bits they support and simply OR'ing them together. While this may not be sufficient in the future as it doesn't consider feature bit dependencies, support for those could be handled at the feature level in the future. This commit moves the `broadcast_node_announcement` function to `PeerHandler` but does not yet implement feature OR'ing.

TheBlueMatt · 2022-09-08T19:50:42Z

Squashed.

TheBlueMatt mentioned this pull request Sep 6, 2022

Onion messages: flip feature bit 🎉 #1688

Merged

TheBlueMatt force-pushed the 2022-08-announcement-rework branch 2 times, most recently from 1887a2e to dbb5885 Compare September 6, 2022 22:48

jkczyz reviewed Sep 6, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

valentinewallace self-requested a review September 7, 2022 01:50

TheBlueMatt force-pushed the 2022-08-announcement-rework branch 3 times, most recently from 8e2fa84 to 1128d85 Compare September 7, 2022 16:48

TheBlueMatt mentioned this pull request Sep 7, 2022

Fetch InitFeatures from both Channel and Routing Message Handlers #1701

Merged

jkczyz reviewed Sep 7, 2022

View reviewed changes

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

jkczyz reviewed Sep 7, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-08-announcement-rework branch 2 times, most recently from 008bd35 to baf5848 Compare September 7, 2022 22:11

valentinewallace reviewed Sep 7, 2022

View reviewed changes

valentinewallace reviewed Sep 8, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-08-announcement-rework branch from a9e7424 to b3547f1 Compare September 8, 2022 17:52

valentinewallace reviewed Sep 8, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2022-08-announcement-rework branch from b3547f1 to 2a064df Compare September 8, 2022 18:47

TheBlueMatt force-pushed the 2022-08-announcement-rework branch from 2a064df to 25a10a1 Compare September 8, 2022 18:50

jkczyz reviewed Sep 8, 2022

View reviewed changes

TheBlueMatt added 3 commits September 8, 2022 19:50

Add a folder to track CHANGELOG entries for the next release

e2495f2

TheBlueMatt force-pushed the 2022-08-announcement-rework branch from a83a363 to e2495f2 Compare September 8, 2022 19:50

jkczyz approved these changes Sep 8, 2022

View reviewed changes

valentinewallace approved these changes Sep 8, 2022

View reviewed changes

TheBlueMatt merged commit ba69536 into lightningdevkit:main Sep 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move broadcast_node_announcement to PeerManager #1699

Move broadcast_node_announcement to PeerManager #1699

TheBlueMatt commented Sep 6, 2022

codecov-commenter commented Sep 6, 2022 •

edited

Loading

TheBlueMatt commented Sep 7, 2022

jkczyz Sep 7, 2022

TheBlueMatt Sep 7, 2022

jkczyz Sep 7, 2022

TheBlueMatt Sep 7, 2022

TheBlueMatt Sep 8, 2022

valentinewallace Sep 7, 2022

TheBlueMatt Sep 8, 2022

valentinewallace Sep 8, 2022

TheBlueMatt Sep 8, 2022

valentinewallace Sep 8, 2022

TheBlueMatt Sep 8, 2022

valentinewallace Sep 8, 2022

TheBlueMatt Sep 8, 2022

valentinewallace left a comment

valentinewallace commented Sep 8, 2022

jkczyz Sep 8, 2022

TheBlueMatt Sep 8, 2022

jkczyz Sep 8, 2022

TheBlueMatt Sep 8, 2022

TheBlueMatt commented Sep 8, 2022

		@@ -0,0 +1,2 @@
		`broadcast_node_announcement` has been moved to `PeerManager` from `ChannelManager`.
		`PeerManager::new`'s new `current_time` argument must be set to the current time, not a counter, for any existing nodes which upgrade.

Move broadcast_node_announcement to PeerManager #1699

Move broadcast_node_announcement to PeerManager #1699

Conversation

TheBlueMatt commented Sep 6, 2022

codecov-commenter commented Sep 6, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Sep 7, 2022

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

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace commented Sep 8, 2022

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 Sep 8, 2022

codecov-commenter commented Sep 6, 2022 •

edited

Loading