Reliably deliver gossip messages from our `ChannelMessageHandler` #3142

TheBlueMatt · 2024-06-24T20:27:22Z

When our ChannelMessageHandler creates gossip broadcast
MessageSendEvents, we generally want these to be reliably
delivered to all our peers, even if there's not much buffer space
available.

Here we do this by passing an extra flag to forward_broadcast_msg
which indicates where the message came from, then ignoring the
buffer-full criteria when the flag is set.

codecov-commenter · 2024-06-24T20:36:14Z

Codecov Report

Attention: Patch coverage is 75.89744% with 47 lines in your changes missing coverage. Please review.

Project coverage is 90.20%. Comparing base (4147de2) to head (116e369).
Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/peer_handler.rs	72.78%	44 Missing and 2 partials ⚠️
lightning/src/util/test_utils.rs	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3142      +/-   ##
==========================================
+ Coverage   89.68%   90.20%   +0.52%     
==========================================
  Files         126      127       +1     
  Lines      103306   110733    +7427     
  Branches   103306   110733    +7427     
==========================================
+ Hits        92651    99889    +7238     
- Misses       7936     8205     +269     
+ Partials     2719     2639      -80

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

shaavan

The code looks great! Just one quick note about the from_chan_handler boolean.

While the commit message clarifies that from_chan_handler suggests it originates from ChannelMessageHandler and should be forwarded to peers, allowing for a larger buffer, this isn’t immediately clear when reading the code alone.

To enhance clarity, we could either:

Add a comment in the code to explain this connection.
Rename from_chan_handler to allow_large_buffer.
Or, rename allow_large_buffer to from_chan_handler.

Option 1 makes the purpose clear right away. I also like option 2, as it succinctly reflects that large buffers are permissible for channel events, as shown here:

for event in chan_events {
    handle_event(event, true);
}

However, if this new boolean is intended only to be used with chan_events, then option 3 would be the most fitting solution.

Let me know what you think. Looking forward to your thoughts!

TheBlueMatt · 2024-06-29T17:22:48Z

Pushed some additional comments/docs.

shaavan

LGTM!
Shall we test to see if this change works as intended?

TheBlueMatt · 2024-08-20T02:30:13Z

Added a test (and rebased, cause this old branch doesn't even build anymore...thanks MSRV-breakages...).

shaavan

LGTM!

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2024-10-01T16:48:45Z

Rebased.

Rather than building a single `Vec` of `MessageSendEvent`s to handle then iterating over them, we move the body of the loop into a lambda and run the loop twice. In some cases, this may save a single allocation, but more importantly it sets us up for the next commit, which needs to know from which handler the `MessageSendEvent` it is processing came from.

valentinewallace

Basically LGTM

lightning/src/ln/peer_handler.rs

valentinewallace · 2024-10-16T19:20:26Z

lightning/src/ln/peer_handler.rs

@@ -1600,7 +1600,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 		}

 		for msg in msgs_to_forward.drain(..) {
-			self.forward_broadcast_msg(&*peers, &msg, peer_node_id.as_ref().map(|(pk, _)| pk));
+			self.forward_broadcast_msg(&*peers, &msg, peer_node_id.as_ref().map(|(pk, _)| pk), false);


It's not really obvious to me why this is false... seems like it could be a channel message at this point?

These come from handling a message from a peer. It could in theory be for one of our channels, but not from our ChannelMessageHandler.

given that this conversation won't live on in the code itself, perhaps a comment or a descriptively named variable for the false might help?

lightning/src/ln/peer_handler.rs

valentinewallace

LGTM after squash

When our `ChannelMessageHandler` creates gossip broadcast `MessageSendEvent`s, we generally want these to be reliably delivered to all our peers, even if there's not much buffer space available. Here we do this by passing an extra flag to `forward_broadcast_msg` which indicates where the message came from, then ignoring the buffer-full criteria when the flag is set.

The `establish_connection` method should work for more than one connection per `PeerManager`, which we fix here.

In testing, its useful to be able to tell the `SocketDescriptor` to pretend the system network buffer is full, which we add here by creating a new `hang_writes` flag. In order to simplify constructing, we also add a new constructor which existing tests are moved to.

This adds a simple test that the gossip message buffer in `PeerManager` is limited, including the new behavior of bypassing the limit when the broadcast comes from the `ChannelMessageHandler`.

TheBlueMatt · 2024-10-17T19:09:20Z

Squashed.

arik-so · 2024-10-22T21:38:40Z

lightning/src/ln/peer_handler.rs

 				}

 				for (node_id, msg) in self.message_handler.custom_message_handler.get_and_clear_pending_msg() {
 					if peers_to_disconnect.get(&node_id).is_some() { continue; }
-					self.enqueue_message(&mut *get_peer_for_forwarding!(&node_id), &msg);
+					self.enqueue_message(&mut *if let Some(peer) = get_peer_for_forwarding!(&node_id) { peer } else { continue; }, &msg);


not really worth voiding the approvals for, but does this have to all be in one line?

rustfmt will figure it out eventually 🤷‍♂️

arik-so · 2024-10-22T21:39:29Z

lightning/src/ln/peer_handler.rs

@@ -1600,7 +1600,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 		}

 		for msg in msgs_to_forward.drain(..) {
-			self.forward_broadcast_msg(&*peers, &msg, peer_node_id.as_ref().map(|(pk, _)| pk));
+			self.forward_broadcast_msg(&*peers, &msg, peer_node_id.as_ref().map(|(pk, _)| pk), false);


given that this conversation won't live on in the code itself, perhaps a comment or a descriptively named variable for the false might help?

arik-so · 2024-10-22T21:43:17Z

lightning/src/ln/peer_handler.rs


 	#[allow(unused_imports)]
 	use crate::prelude::*;

 	#[derive(Clone)]
 	struct FileDescriptor {
 		fd: u16,
+		hang_writes: Arc<AtomicBool>,
 		outbound_data: Arc<Mutex<Vec<u8>>>,


I think this could use a comment, particularly pointing out its purpose is primarily for testing

The entire code block is inside #[cfg(test)], I think its pretty obvious its only for testing :)

shaavan reviewed Jun 25, 2024

View reviewed changes

shaavan reviewed Jul 5, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-06-robust-updates branch from 9b88a14 to 8733e70 Compare August 20, 2024 02:30

shaavan reviewed Aug 20, 2024

View reviewed changes

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

TheBlueMatt force-pushed the 2024-06-robust-updates branch from 8733e70 to d328d41 Compare August 20, 2024 14:59

TheBlueMatt force-pushed the 2024-06-robust-updates branch from d328d41 to 4f6a09c Compare October 1, 2024 16:48

TheBlueMatt force-pushed the 2024-06-robust-updates branch from 4f6a09c to e483e53 Compare October 1, 2024 16:49

valentinewallace reviewed Oct 16, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-06-robust-updates branch from e483e53 to 116e369 Compare October 17, 2024 00:19

valentinewallace reviewed Oct 17, 2024

View reviewed changes

TheBlueMatt added 4 commits October 17, 2024 19:09

Fix the establish_connection utility method for a 2nd connection

bcad257

The `establish_connection` method should work for more than one connection per `PeerManager`, which we fix here.

Add a test of gossip message buffer limiting in PeerManager

f4b2311

This adds a simple test that the gossip message buffer in `PeerManager` is limited, including the new behavior of bypassing the limit when the broadcast comes from the `ChannelMessageHandler`.

TheBlueMatt force-pushed the 2024-06-robust-updates branch from 116e369 to f4b2311 Compare October 17, 2024 19:09

valentinewallace approved these changes Oct 17, 2024

View reviewed changes

shaavan approved these changes Oct 18, 2024

View reviewed changes

arik-so reviewed Oct 22, 2024

View reviewed changes

TheBlueMatt merged commit 3c868dd into lightningdevkit:main Oct 23, 2024
18 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reliably deliver gossip messages from our `ChannelMessageHandler` #3142

Reliably deliver gossip messages from our `ChannelMessageHandler` #3142

TheBlueMatt commented Jun 24, 2024

codecov-commenter commented Jun 24, 2024 •

edited by codecov bot

Loading

shaavan left a comment

TheBlueMatt commented Jun 29, 2024

shaavan left a comment

TheBlueMatt commented Aug 20, 2024

shaavan left a comment

TheBlueMatt commented Oct 1, 2024

valentinewallace left a comment

valentinewallace Oct 16, 2024

TheBlueMatt Oct 17, 2024

arik-so Oct 22, 2024

valentinewallace left a comment

TheBlueMatt commented Oct 17, 2024

arik-so Oct 22, 2024

TheBlueMatt Oct 23, 2024

arik-so Oct 22, 2024

arik-so Oct 22, 2024

TheBlueMatt Oct 23, 2024

Reliably deliver gossip messages from our ChannelMessageHandler #3142

Reliably deliver gossip messages from our ChannelMessageHandler #3142

Conversation

TheBlueMatt commented Jun 24, 2024

codecov-commenter commented Jun 24, 2024 • edited by codecov bot Loading

Codecov Report

shaavan left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 29, 2024

shaavan left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 20, 2024

shaavan left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 1, 2024

valentinewallace 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

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 17, 2024

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

Reliably deliver gossip messages from our `ChannelMessageHandler` #3142

Reliably deliver gossip messages from our `ChannelMessageHandler` #3142

codecov-commenter commented Jun 24, 2024 •

edited by codecov bot

Loading