-
Notifications
You must be signed in to change notification settings - Fork 781
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
Disable flood publishing #4383
Disable flood publishing #4383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but would like a 2nd opinion from @pawanjay176
Yeah. It might be worth holding this off from the next release. We can potentially merge this if the issue becomes severe after the next release. |
e3ba5d7
to
fd36bf5
Compare
what if we put this behind a CLI flag and merged it to unstable? |
Agree with this. Would be nice to toggle flood sub to compare. |
Yeah. I'll do that. I'll make a hidden CLI flag. We probably want to increase the mesh size a little if we are not flood publishing. |
Hey @michaelsproul @pawanjay176 We are doing a bunch of network changes. What do you guys think about getting this in also? This disables flood publishing (because we don't really have a soln for handling it) and it increases the default mesh size to somewhat account for the lack of flood publishing. I also increased the heartbeat to 1s, to avoid some excessive gossip messaging. If you guys agree, lets get this in. If the network shows any signs of degredation we can make a patch release. |
Seems ok to me. I guess a patch release isn't much more work than telling everyone to add a flag like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay to me as well since the patch is easy if things don't work out
This PR is aimed to improve message latency.
The network is seeing delays in message propagation. We suspect gossipsub flood publishing to be contributing to this.
A interim solution has been suggested in gossipsub: libp2p/rust-libp2p#3666 however further testing and analysis is required before moving down that path.
There can be gains in message latency in a number of areas, including improving the transport, introducing backpressure and adding more sophisticated logic around sending messages with known bandwidth limitations.
The recent change in the network subnet structure is going to push lighthouse to maintain a higher peer count. If flood publishing is correctly attributing to the message latency, I think its wise we temporarily disable it, especially as our peer count grows, whilst we explore improvements in other layers of libp2p.