Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-broadcast channel_announcements every six blocks for a week #3360

Conversation

TheBlueMatt
Copy link
Collaborator

When we first get a public channel confirmed at six blocks, we broadcast a channel_announcement once and then move on. As long as it makes it into our local network graph that should be okay, as we should send peers our network graph contents as they seek to sync, however its possible an ill-timed shutdown could cause this to fail, and relying on peers to do a full historical sync from us may delay channel_announcement propagation.

Instead, here, we re-broadcast our channel_announcements every six blocks for a week, which should be way more than robust enough to get them properly across the P2P network.

Fixes #2418

@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Oct 13, 2024
Copy link

codecov bot commented Oct 13, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.65%. Comparing base (ad19d93) to head (2024c84).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 95.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3360      +/-   ##
==========================================
+ Coverage   89.61%   89.65%   +0.04%     
==========================================
  Files         127      127              
  Lines      103533   104244     +711     
  Branches   103533   104244     +711     
==========================================
+ Hits        92785    93464     +679     
- Misses       8051     8104      +53     
+ Partials     2697     2676      -21     

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

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM, but looks like that the mutation are failing? but I have no enough knowledge of them to spoof the error if any

Found 16 mutants to test
 INFO Auto-set test timeout to 2m 7s
ok       Unmutated baseline in 203.3s build + 25.3s test
caught   lightning/src/ln/channelmanager.rs:10174:3: replace ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event with () in 251.4s build + 16.8s test
MISSED   lightning/src/ln/channelmanager.rs:10224:64: replace || with && in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 359.2s build + 33.2s test
MISSED   lightning/src/ln/channelmanager.rs:10224:87: replace < with == in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 306.2s build + 56.6s test
MISSED   lightning/src/ln/channelmanager.rs:10224:87: replace < with > in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 321.2s build + 40.6s test
caught   lightning/src/ln/channelmanager.rs:10224:96: replace + with - in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 290.8s build + 17.6s test
MISSED   lightning/src/ln/channelmanager.rs:10224:96: replace + with * in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 298.9s build + 51.1s test
MISSED   lightning/src/ln/channelmanager.rs:10229:26: replace &= with |= in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in [296](https://github.com/lightningdevkit/rust-lightning/actions/runs/11316285439/job/31468424205?pr=3360#step:5:297).2s build + 57.1s test
caught   lightning/src/ln/channelmanager.rs:10229:26: replace &= with ^= in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 299.0s build + 40.2s test
caught   lightning/src/ln/channelmanager.rs:10229:57: replace || with && in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 293.0s build + 45.6s test
MISSED   lightning/src/ln/channelmanager.rs:10229:84: replace == with != in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 310.4s build + 55.1s test
MISSED   lightning/src/ln/channelmanager.rs:10229:80: replace % with / in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 310.4s build + 56.2s test
MISSED   lightning/src/ln/channelmanager.rs:10229:80: replace % with + in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 291.4s build + 46.5s test
MISSED   lightning/src/ln/channelmanager.rs:10229:94: replace % with / in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in [299](https://github.com/lightningdevkit/rust-lightning/actions/runs/11316285439/job/31468424205?pr=3360#step:5:300).1s build + 56.1s test
MISSED   lightning/src/ln/channelmanager.rs:10229:94: replace % with + in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 305.0s build + 45.3s test
caught   lightning/src/ln/channelmanager.rs:10235:27: replace &= with |= in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 293.7s build + 35.7s test
caught   lightning/src/ln/channelmanager.rs:10235:27: replace &= with ^= in ChannelManager<M, T, ES, NS, SP, F, R, MR, L>::do_chain_event in 283.5s build + 17.0s test
16 mutants tested in 50m 26s: 10 missed, 6 caught

@TheBlueMatt
Copy link
Collaborator Author

Yea, we currently don't enforce mutation tests passing - they're really informational for reviewers to detect where there's missing test coverage if they feel test coverage is merited.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
// re-broadcast every hour (6 blocks) after the initial
// broadcast, or if this is the first time we're ready to
// broadcast this channel.
should_announce &= announcement_sigs.is_some() || funding_conf_height % 6 == height % 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we first learn about a channel a block after it confirms, we will not propagate that gossip. I take it that's acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, you're immediately reassigning the boolean after defining it. Why not do it in one assignment, or do something like:

let meets_height_requirements = funding_conf_height < height + 1008 && funding_conf_height % 6 == height % 6;
let should_announce = announcement_sigs.is_some() || meets_height_requirements;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we first learn about a channel a block after it confirms, we will not propagate that gossip. I take it that's acceptable?

We should under the announcement_sigs.is_some() condition, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, sure, though it's hidden behind multiple layers of indirection through closure passing, sorry

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
arik-so
arik-so previously approved these changes Oct 16, 2024
@TheBlueMatt
Copy link
Collaborator Author

Squashed.

@TheBlueMatt TheBlueMatt force-pushed the 2024-10-rebroadcast-chan-announcements branch from 070d816 to 9a6abfd Compare October 16, 2024 21:03
@arik-so
Copy link
Contributor

arik-so commented Oct 17, 2024

can you add an allow unused mut so CI passes?

@TheBlueMatt
Copy link
Collaborator Author

Ugh sorry

@valentinewallace
Copy link
Contributor

Looks like ubuntu-latest 1.63 is still failing for the unused mut?

When we first get a public channel confirmed at six blocks, we
broadcast a `channel_announcement` once and then move on. As long
as it makes it into our local network graph that should be okay, as
we should send peers our network graph contents as they seek to
sync, however its possible an ill-timed shutdown could cause this
to fail, and relying on peers to do a full historical sync from us
may delay `channel_announcement` propagation.

Instead, here, we re-broadcast our `channel_announcement`s every
six blocks for a week, which should be way more than robust enough
to get them properly across the P2P network.

Fixes lightningdevkit#2418
@TheBlueMatt TheBlueMatt force-pushed the 2024-10-rebroadcast-chan-announcements branch from 2024c84 to 948f179 Compare October 29, 2024 17:53
@TheBlueMatt
Copy link
Collaborator Author

Grrr

$ git diff-tree -U2 2024c848f 948f1794a
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index a2237ed25..e0b8bcd7a 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -10228,5 +10228,5 @@ where
 									let rebroadcast_announcement = funding_conf_height < height + 1008
 										&& funding_conf_height % 6 == height % 6;
-									#[allow(unused_mut)]
+									#[allow(unused_mut, unused_assignments)]
 									let mut should_announce = announcement_sigs.is_some() || rebroadcast_announcement;
 									// Most of our tests were written when we only broadcasted

@valentinewallace
Copy link
Contributor

Windows-latest failure is #3114

@valentinewallace valentinewallace merged commit 76a93a3 into lightningdevkit:main Nov 4, 2024
17 of 20 checks passed
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.

ChannelAnnouncement rebroadcasting is broken
4 participants