Skip to content

Send BroadcastChannelAnnouncements via the broadcast queue#4508

Open
TheBlueMatt wants to merge 1 commit intolightningdevkit:mainfrom
TheBlueMatt:2026-03-broadcast-via-broadcast-queue
Open

Send BroadcastChannelAnnouncements via the broadcast queue#4508
TheBlueMatt wants to merge 1 commit intolightningdevkit:mainfrom
TheBlueMatt:2026-03-broadcast-via-broadcast-queue

Conversation

@TheBlueMatt
Copy link
Collaborator

In 47a3e5c 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

In 47a3e5c 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 lightningdevkit#4437

Written by Claude
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 24, 2026

I've assigned @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@TheBlueMatt
Copy link
Collaborator Author

cc @tankyleo @tnull does this potentially explain the issues y'all saw?

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: double space — "cannot be announced" → "cannot be announced".

Suggested change
// Note that announcement_signatures fails if the channel cannot be announced,
// Note that announcement_signatures fails if the channel cannot be announced,

@ldk-claude-review-bot
Copy link
Collaborator

Review Summary

The core change — moving BroadcastChannelAnnouncement from per-peer queues to pending_broadcast_messages — is sound. Lock ordering is consistent across all access sites (per_peer_state > peer_state_mutex > pending_broadcast_messages). The test reordering in priv_short_conf_tests.rs correctly reflects the new event collection order in get_and_clear_pending_msg_events (per-peer events first, then broadcast events appended).

Inline comments posted

  • lightning/src/ln/channelmanager.rs:13008 — Minor typo: double space in comment ("cannot be announced")

No bugs or security issues found

The change correctly fixes the scenario described in #4437 where BroadcastChannelAnnouncement events generated during block connections while a peer was disconnected would remain in the per-peer queue and trigger the debug_assert!(peer_state.pending_msg_events.is_empty()) on peer reconnect.

@codecov
Copy link

codecov bot commented Mar 24, 2026

Codecov Report

❌ Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.19%. Comparing base (b3a99f6) to head (fb7875f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 80.95% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4508      +/-   ##
==========================================
- Coverage   86.19%   86.19%   -0.01%     
==========================================
  Files         160      160              
  Lines      107537   107545       +8     
  Branches   107537   107545       +8     
==========================================
+ Hits        92693    92695       +2     
- Misses      12218    12225       +7     
+ Partials     2626     2625       -1     
Flag Coverage Δ
tests 86.19% <80.95%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tnull
Copy link
Contributor

tnull commented Mar 24, 2026

cc @tankyleo @tnull does this potentially explain the issues y'all saw?

Yes, that might very well be it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No-pending-messages debug_assert is likely race-y

4 participants