Reliably deliver gossip messages from our ChannelMessageHandler
#3142
Conversation
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.
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.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3142 +/- ##
=======================================
Coverage ? 90.35%
=======================================
Files ? 122
Lines ? 105863
Branches ? 105863
=======================================
Hits ? 95651
Misses ? 7416
Partials ? 2796 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
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_handlertoallow_large_buffer. - Or, rename
allow_large_buffertofrom_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!
|
Pushed some additional comments/docs. |
When our
ChannelMessageHandlercreates gossip broadcastMessageSendEvents, we generally want these to be reliablydelivered 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_msgwhich indicates where the message came from, then ignoring the
buffer-full criteria when the flag is set.