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

Send a gossip_timestamp_filter on connect to enable gossip sync #1368

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

On connection, if our peer supports gossip queries, and we never
send a gossip_timestamp_filter, our peer is supposed to never
send us gossip outside of explicit queries. Thus, we'll end up
always having stale gossip information after the first few
connections we make to peers.

The solution is to send a dummy gossip_timestamp_filter
immediately after connecting to peers.

Its somewhat strange to have a trait method which is named after
the intended action, rather than the action that occurred, leaving
it up to the implementor what action they want to take.
@TheBlueMatt TheBlueMatt added this to the 0.0.106 milestone Mar 17, 2022
On connection, if our peer supports gossip queries, and we never
send a `gossip_timestamp_filter`, our peer is supposed to never
send us gossip outside of explicit queries. Thus, we'll end up
always having stale gossip information after the first few
connections we make to peers.

The solution is to send a dummy `gossip_timestamp_filter`
immediately after connecting to peers.
@TheBlueMatt TheBlueMatt force-pushed the 2022-03-fix-post-start-sync branch from 28feb19 to 3cca221 Compare March 17, 2022 22:18
@codecov-commenter
Copy link

Codecov Report

Merging #1368 (3cca221) into main (c244c78) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #1368      +/-   ##
==========================================
- Coverage   90.73%   90.71%   -0.02%     
==========================================
  Files          73       73              
  Lines       40808    40824      +16     
  Branches    40808    40824      +16     
==========================================
+ Hits        37027    37035       +8     
- Misses       3781     3789       +8     
Impacted Files Coverage Δ
lightning/src/ln/channelmanager.rs 84.75% <0.00%> (-0.03%) ⬇️
lightning/src/ln/msgs.rs 85.84% <ø> (ø)
lightning/src/util/events.rs 32.86% <0.00%> (-0.59%) ⬇️
lightning/src/ln/peer_handler.rs 48.30% <25.00%> (-0.11%) ⬇️
lightning/src/routing/network_graph.rs 89.53% <94.73%> (+0.01%) ⬆️
lightning-net-tokio/src/lib.rs 76.69% <100.00%> (ø)
lightning/src/util/test_utils.rs 82.42% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.07% <0.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c244c78...3cca221. Read the comment docs.

@jkczyz jkczyz self-requested a review March 17, 2022 23:31
@valentinewallace
Copy link
Contributor

Looks good. Did we check that this fixes the issue?

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure why CI is sad.

@TheBlueMatt
Copy link
Collaborator Author

Kicked CI - it said "failed" but didn't have logs at all so I assume GitHub Actions is just acting up like usual.

@TheBlueMatt
Copy link
Collaborator Author

Looks good. Did we check that this fixes the issue?

No idea, I didn't bother to reproduce the issue, but the spec seems to be pretty clear that this is required. It's possible we have other issues as well, though.

@TheBlueMatt TheBlueMatt merged commit 3920157 into lightningdevkit:main Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants