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.

Sign up for GitHub

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

Jump to bottom

peer: deprioritize gossip traffic #2690

Merged
merged 6 commits into from
Mar 6, 2019
Merged

peer: deprioritize gossip traffic #2690

merged 6 commits into from
Mar 6, 2019
+122 −124

Conversation

cfromknecht
Copy link
Contributor

Should fix #2686

@cfromknecht cfromknecht mentioned this pull request Feb 23, 2019
Closed
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour p2p Code related to the peer-to-peer behaviour discovery Peer and route discovery / whisper protocol related issues/PRs server labels Feb 23, 2019
@Roasbeef Roasbeef added this to the 0.6 milestone Feb 23, 2019
@Roasbeef Roasbeef requested a review from wpaulino February 24, 2019 19:38
@NicolasDorier NicolasDorier mentioned this pull request Feb 25, 2019
Closed
@cfromknecht cfromknecht mentioned this pull request Mar 4, 2019
Closed
wpaulino
wpaulino reviewed Mar 5, 2019
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

The only other case within the gossiper where we send messages to peers is in the reliable sender, but I think those can remain as is as there shouldn't be that many per peer.

@cfromknecht cfromknecht force-pushed the hwsc-fndg-priority-queue branch from 8e1aac4 to 935ea7d Compare March 6, 2019 01:09
wpaulino
wpaulino approved these changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM 🕸

@cfromknecht
Copy link
Contributor Author

The only other case within the gossiper where we send messages to peers is in the reliable sender, but I think those can remain as is as there shouldn't be that many per peer.

Indeed, i think we want these to be prioritized also so that we can remove them from the message store as quickly as possible

@Roasbeef
Copy link
Member

Roasbeef commented Mar 6, 2019

Agreed that we'd want the direct chan ann/sig/update messages to be sent in prio mode.

Roasbeef
Roasbeef approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 💥

Did a connect+openchannel test right after a restart on one of my testnet nodes and also a connect+openchannel with a fresh channel graph. Not a precise measures, but both tests were noticeably snappier (returned from the command line sooner) with this patch.

@Roasbeef Roasbeef merged commit c853555 into lightningnetwork:master Mar 6, 2019
@cfromknecht cfromknecht deleted the hwsc-fndg-priority-queue branch March 6, 2019 05:50
This was referenced Apr 9, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@wpaulino wpaulino wpaulino approved these changes

Assignees
No one assigned
Labels
discovery Peer and route discovery / whisper protocol related issues/PRs enhancement Improvements to existing features / behaviour p2p Code related to the peer-to-peer behaviour server
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.

lnd outgoing gossip flood can cause message timeouts?
3 participants
@cfromknecht @Roasbeef @wpaulino