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

Make channel prioritization based on number of messages in channel (vs number of bytes sent) #2954

Open
Tracked by #3053
ValarDragon opened this issue Apr 30, 2024 · 3 comments
Labels
enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team.

Comments

@ValarDragon
Copy link
Collaborator

ValarDragon commented Apr 30, 2024

Feature Request

Summary

Channel.RecentlySent is used for choosing priority of a channel. Currently channel selection is done as channel with the minimum channel.RecentlySent / channel.Priority ratio. Every packet message increases channel.RecentlySent by the proto-encoded packet size. channel.RecentlySent gets lowered by 20% at every UpdateStats window (which is just a 2 second timer).

BlockGossip is consensus time critical, and has very large packets. Its current priority weight is 10, vs vote's 7, hasVote's 1, and mempool's 5. Its packets are block parts which are large. This guarantees block gossip will always be the lowest prioritized packet out of all packets, since its recently sent will always be very large (in the 100kb's), which is problematic for fast block gossip. Any of the vote messages or mempool messages to a peer will outcompete block part gossip in the prioritization.

This matches behavior observed that we never hit our bandwidth flow rates in production and that block gossip just occurs slowly.

Instead channel.RecentlySent should first off be based on the number of packets on a channel, not the bandwidth size.

Problem Definition

This should greatly increase the prioritization of block gossip.

Proposal

channel.RecentlySent should first off be based on the number of packets on a channel, not the bandwidth size.

@ValarDragon ValarDragon added enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team. labels Apr 30, 2024
@cason
Copy link
Contributor

cason commented May 10, 2024

Can you please re-phrase the issue focusing on the problem that we want to solve, or the feature we want to implement, instead of the implementation steps/change/details? While this part is also important, and should be included, for a general reader would be interesting to understand the actual proposal here.

@cason
Copy link
Contributor

cason commented May 10, 2024

I am also wondering whether this is a " Protocol change proposal " instead of a feature request, as this change would impact the whole operation of the p2p layer.

@ValarDragon ValarDragon changed the title Change Channel.RecentlySent to be number of messages, not number of bytes Make channel de-prioritization based on number of messages in channel (rather than number of bytes sent on channel) May 13, 2024
@ValarDragon ValarDragon changed the title Make channel de-prioritization based on number of messages in channel (rather than number of bytes sent on channel) Make channel prioritization based on number of messages in channel (vs number of bytes sent) May 13, 2024
@nmarcetic
Copy link

This matches the behavior observed that we never hit our bandwidth flow rates in production and that block gossip just occurs slowly.

Makes sense; We had the same behavior (neither bandwidth nor size rates).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants