Send BroadcastChannelAnnouncements via the broadcast queue
What the developer wrote
Authored by Matt Corallo
Send BroadcastChannelAnnouncements via the broadcast queue
In 47a3e5c694321dac1a1d0f53e6dcb357282a79be we started asserting
that the per-peer message queue was empty when a peer connected to
ensure we don't have stale messages sitting around in memory. This
turned up an issue for `channel_announcement` messages generated by
block connections while a peer was disconnected.
Here we push those out through the broadcast message queue rather
than the per-peer message queue as there's no reason to tie them to
the individual peer anyway, fixing the assertions.
This should fix #4437
Written by Claude
What changed, and why it matters
This commit fixes an internal consistency bug in LDK (a Lightning Network implementation in Rust). When a new block created a channel announcement while a peer was offline, the announcement was placed in that specific peer's queue. After a later change started checking that peer queues are empty on reconnect, this leftover message caused assertion failures (crashes in debug builds). The fix moves these broadcast messages into a global broadcast queue, where they belong, instead of a per-peer queue.
Review and merge if tests pass. The change is defensive and corrects message routing. Monitor issue #4437 for any follow-up reports. No immediate security advisory appears necessary, but the crash-on-reconnect behavior could be considered a low-severity DoS in debug builds.
Security signals we found
Fixes assertion failure / potential panic on peer reconnection (denial-of-service vector)
Moves broadcast messages from per-peer state to global broadcast queue, reducing stale-state risk
Changes behavior of message delivery ordering observable by tests
References GitHub issue #4437 as the bug being fixed
Evidence from the diff
The patch changes how BroadcastChannelAnnouncement events are stored. Previously, when a block connection generated a signed channel announcement (after announcement_signatures exchange), the event was pushed into peer_state.pending_msg_events. After commit 47a3e5c6 added a debug assertion that per-peer queues are empty on peer connection, these stale announcements triggered crashes. The fix routes both the block-connection path and the announcement_signatures path through self.pending_broadcast_messages, which is the correct queue for messages intended for all peers. A debug assertion is also updated to flag any BroadcastChannelAnnouncement found in a per-peer queue. Test expectations are adjusted because broadcast events now appear in a different order relative to per-peer events.
Changed components
lightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/priv_short_conf_tests.rsInspect captured patch +29 / −28
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index d042a69..f9772bb 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -13004,12 +13004,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
msg,
&self.config.read().unwrap(),
);
- peer_state.pending_msg_events.push(MessageSendEvent::BroadcastChannelAnnouncement {
- msg: try_channel_entry!(self, peer_state, res, chan_entry),
- // Note that announcement_signatures fails if the channel cannot be announced,
- // so get_channel_update_for_broadcast will never fail by the time we get here.
- update_msg: Some(self.get_channel_update_for_broadcast(chan).unwrap().0),
- });
+ let announcement_msg = try_channel_entry!(self, peer_state, res, chan_entry);
+ // Note that announcement_signatures fails if the channel cannot be announced,
+ // so get_channel_update_for_broadcast will never fail by the time we get here.
+ let update_msg = self.get_channel_update_for_broadcast(chan).unwrap().0;
+ self.pending_broadcast_messages.lock().unwrap().push(
+ MessageSendEvent::BroadcastChannelAnnouncement {
+ msg: announcement_msg,
+ update_msg: Some(update_msg),
+ },
+ );
} else {
return try_channel_entry!(self, peer_state, Err(ChannelError::close(
"Got an announcement_signatures message for an unfunded channel!".into())), chan_entry);
@@ -15485,11 +15489,14 @@ impl<
&MessageSendEvent::HandleError { .. } => false,
// Gossip
&MessageSendEvent::SendChannelAnnouncement { .. } => false,
- &MessageSendEvent::BroadcastChannelAnnouncement { .. } => true,
- // [`ChannelManager::pending_broadcast_events`] holds the [`BroadcastChannelUpdate`]
- // This check here is to ensure exhaustivity.
+ // [`ChannelManager::pending_broadcast_messages`] holds broadcast events,
+ // not per-peer queues.
+ &MessageSendEvent::BroadcastChannelAnnouncement { .. } => {
+ debug_assert!(false, "BroadcastChannelAnnouncement should be in pending_broadcast_messages");
+ false
+ },
&MessageSendEvent::BroadcastChannelUpdate { .. } => {
- debug_assert!(false, "This event shouldn't have been here");
+ debug_assert!(false, "BroadcastChannelUpdate should be in pending_broadcast_messages");
false
},
&MessageSendEvent::BroadcastNodeAnnouncement { .. } => true,
@@ -15687,10 +15694,6 @@ impl<
/// the chunks of `MessageSendEvent`s for different peers is random. I.e. if the array contains
/// `MessageSendEvent`s for both `node_a` and `node_b`, the `MessageSendEvent`s for `node_a`
/// will randomly be placed first or last in the returned array.
- ///
- /// Note that even though `BroadcastChannelAnnouncement` and `BroadcastChannelUpdate`
- /// `MessageSendEvent`s are intended to be broadcasted to all peers, they will be placed among
- /// the `MessageSendEvent`s to the specific peer they were generated under.
fn get_and_clear_pending_msg_events(&self) -> Vec<MessageSendEvent> {
let events = RefCell::new(Vec::new());
PersistenceNotifierGuard::optionally_notify(self, || {
@@ -16143,14 +16146,16 @@ impl<
if let Some(announcement) = funded_channel.get_signed_channel_announcement(
&self.node_signer, self.chain_hash, height, &self.config.read().unwrap(),
) {
- pending_msg_events.push(MessageSendEvent::BroadcastChannelAnnouncement {
- msg: announcement,
- // Note that get_signed_channel_announcement fails
- // if the channel cannot be announced, so
- // get_channel_update_for_broadcast will never fail
- // by the time we get here.
- update_msg: Some(self.get_channel_update_for_broadcast(funded_channel).unwrap().0),
- });
+ self.pending_broadcast_messages.lock().unwrap().push(
+ MessageSendEvent::BroadcastChannelAnnouncement {
+ msg: announcement,
+ // Note that get_signed_channel_announcement
+ // fails if the channel cannot be announced, so
+ // get_channel_update_for_broadcast will never
+ // fail by the time we get here.
+ update_msg: Some(self.get_channel_update_for_broadcast(funded_channel).unwrap().0),
+ },
+ );
}
}
}
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 80274d1..e885949 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -1121,10 +1121,6 @@ pub fn get_htlc_update_msgs(node: &Node, recipient: &PublicKey) -> msgs::Commitm
/// Fetches the first `msg_event` to the passed `node_id` in the passed `msg_events` vec.
/// Returns the `msg_event`.
-///
-/// Note that even though `BroadcastChannelAnnouncement` and `BroadcastChannelUpdate`
-/// `msg_events` are stored under specific peers, this function does not fetch such `msg_events` as
-/// such messages are intended to all peers.
pub fn remove_first_msg_event_to_node(
msg_node_id: &PublicKey, msg_events: &mut Vec<MessageSendEvent>,
) -> MessageSendEvent {
diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs
index ffe5ea6..70d5853 100644
--- a/lightning/src/ln/priv_short_conf_tests.rs
+++ b/lightning/src/ln/priv_short_conf_tests.rs
@@ -255,7 +255,7 @@ fn do_test_1_conf_open(connect_style: ConnectStyle) {
assert_eq!(bs_announce_events.len(), 2);
let bs_announcement_sigs =
if let MessageSendEvent::SendAnnouncementSignatures { ref node_id, ref msg } =
- bs_announce_events[1]
+ bs_announce_events[0]
{
assert_eq!(*node_id, node_a_id);
msg.clone()
@@ -264,7 +264,7 @@ fn do_test_1_conf_open(connect_style: ConnectStyle) {
};
let (bs_announcement, bs_update) =
if let MessageSendEvent::BroadcastChannelAnnouncement { ref msg, ref update_msg } =
- bs_announce_events[0]
+ bs_announce_events[1]
{
(msg.clone(), update_msg.clone().unwrap())
} else {
Why this scored 33/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.