-
-
Notifications
You must be signed in to change notification settings - Fork 318
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
Implement rate limit #3454
Implement rate limit #3454
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3454 +/- ##
==========================================
- Coverage 37.90% 37.64% -0.27%
==========================================
Files 308 310 +2
Lines 8120 8216 +96
Branches 1247 1262 +15
==========================================
+ Hits 3078 3093 +15
- Misses 4894 4975 +81
Partials 148 148 |
Code Climate has analyzed commit 2d60bf1 and detected 2 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Performance Report✔️ no performance regression detected Full benchmark results
|
8f58fdd
to
5eec08f
Compare
I'm concerned about the memory and performance cost of this implementation.
|
Update with some analysis and benchmark tests: Memory
Performance
@dapplion please let me know if you have any concern, or any other tests that I can add |
packages/lodestar/test/unit/network/reqresp/response/rateLimiter.test.ts
Show resolved
Hide resolved
packages/lodestar/test/unit/network/reqresp/response/rateLimiter.test.ts
Outdated
Show resolved
Hide resolved
38736a7
to
5962dc6
Compare
This solution looks dangerous assuming 5 seconds isn't safe. What if the node is very overloaded and pormises get stuck for a long time in the event loop? What do you think about implementing the solution I proposed offline:
|
eeb353c
to
2d60bf1
Compare
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.
This LGTM but I've got a question before merging.
If we only have an inbound rate limiter, don't we run the risk of disconnecting from ourselves on the network?
Should we also have the outbound rate limiter implemented before merging this?
@wemeetagain given the threshold of 500 blocks per minute, I think the issue may only happen on a very small devnet, it's safe for prater/pyrmont/mainnet, even if we have the issue we can always tweak through cli params, also we have #3344. What's your thought @dapplion ? |
I think it's good, we can fix latter if we see issues |
Motivation
We want to apply rate limit when we receivebeacon_blocks_by_range
andbeacon_blocks_by_root
requestsDescription
RateTracker
class that check the rate limit of request count and block countrequestCountPeerLimit, blockCountPeerLimit
and totalrequestCountTotalLimit, blockCountTotalLimit
total
params 4x thepeer
paramsNote
We will need to apply
RateTracker
when we issue rpc block requests too or we'll get banned by other peers, see #3344Closes #3451