-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Rate limit outgoing gossip bandwidth by peer #10103
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
base: master
Are you sure you want to change the base?
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.
Summary of Changes
Hello @yyforyongyu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new per-peer rate limiting mechanism for outgoing gossip messages. This enhancement addresses a potential issue where a single misbehaving or very active peer could consume a disproportionate amount of the node's outgoing bandwidth, impacting overall network communication. By applying a dedicated rate limit to each peer, the system ensures more equitable distribution of gossip bandwidth and improves network stability.
Highlights
- Configurable Per-Peer Rate Limiting: A new configuration option,
gossip.peer-msg-rate-bytes
, has been added. This allows operators to specify the maximum outgoing gossip bandwidth allocated to each individual peer, complementing the existing global rate limit. - Dedicated Peer-Level Rate Limiters: Each
GossipSyncer
now maintains its ownrate.Limiter
instance. This ensures that outgoing messages to a specific peer adhere to the newly introduced per-peer bandwidth limits, preventing a single peer from monopolizing gossip bandwidth. - Refactored Rate Limiting Logic: Common rate limiting functions (
deriveRateLimitReservation
,waitMsgDelay
,maybeRateLimitMsg
) have been extracted fromSyncManager
into standalone, reusable functions. This enables their application to both the global gossip rate limiter and the new per-peer rate limiters.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces a per-peer rate limit for outgoing gossip messages, which is a valuable addition to prevent a single peer from consuming all available bandwidth. The implementation reuses existing rate-limiting logic and includes new configuration options, updates to the gossip syncer, and documentation.
I've identified a critical bug in the new message sending logic that would cause messages to be sent multiple times, along with medium-severity issues related to style guide violations and documentation clarity.
4018223
to
72f1433
Compare
72f1433
to
8e41ecf
Compare
// DefaultPeerMsgBytesPerSecond is the max bytes/s we'll permit for | ||
// outgoing messages for a single peer. Once tokens (bytes) have been | ||
// taken from the bucket, they'll be refilled at this rate. | ||
DefaultPeerMsgBytesPerSecond = 50 * 1_024 |
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.
What about scaling this based on the total global value? So something like: a peer can only use up to 5% of the global limit?
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.
yeah make sense. Tho I think we may still need an absolute value, similar to how we handle the budget used in the sweeper. The idea here is to limit the per-peer bandwidth, assuming only one or two bad peers exist, like what we've seen from the logs. For large nodes, they can have hundreds of peers, so 5% may be too large. And for small nodes, they may have less than 10 peers, and 5% can be too conservative.
8e41ecf
to
5987174
Compare
@yyforyongyu, remember to re-request review from reviewers when ready |
This allows us to reuse them in the upcoming commits where we introduce a rate limiter to the gossip syncer.
5987174
to
d64a123
Compare
We now add another layer of rate limiting before sending the messages inside `GossipSyncer`.
d64a123
to
2450d07
Compare
Sometimes a single peer can easily use up all the bandwidth, as suggested by this log, where
125172
msgs were sent to reply to a single query,This PR now puts another rate limiter on the peer-level, which turns out to be easy to implement given we can reuse the methods created for the sync manager.