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

Simplify Ethereum RPC rate limiting #596

Merged
merged 11 commits into from Dec 19, 2019

Conversation

@albrow
Copy link
Member

albrow commented Dec 16, 2019

This PR simplifies our implementation of Ethereum RPC rate limiting in order to address some potential bugs and make it easier to test. In general, it also loosens the restrictions of the rate limiter to make it less likely to affect non-spammers under normal network conditions.

This is most likely a fix for #585.

Summary of changes:

  1. Removed the rate.Limiter that was used to enforce ETHEREUM_RPC_MAX_REQUESTS_PER_24_HR_UTC. This limit is now enforced via a simple counter which is reset every UTC midnight. This greatly simplifies our implementation and makes it easier to write tests. It also reduces the chances of artificially limiting the rate of requests for non-spammers (especially around UTC midnight). But it does come with the downside of not guaranteeing that RPC requests will be evenly distributed throughout the 24 hour period.
  2. Removed the buffer for maxRequestsPer24Hrs. The consequences for exceeding this limit are usually not severe, so we feel the buffer is no longer necessary.
  3. Removed the lowestPossibleMaxRequestsPer24Hrs constraint. We already have a check in the core package to prevent you from setting ETHEREUM_RPC_MAX_REQUESTS_PER_24_HR_UTC too low based on BLOCK_POLLING_INTERVAL.
  4. Added a small burst to the per second rate.Limiter. Currently set to maxRequestsPerSecond/2. IMO the old implementation which set the burst to 1 was too restrictive. It guaranteed that we would never exceed maxRequestsPerSecond for any one second period but also enforced a minimum delay of 1/maxRequestsPerSecond seconds between subsequent requests. This delay was enforced even after long periods of not sending any requests. This works out to 30ms with the default settings, which is nearly twice the median response time for some Ethereum hosting providers (our Alchemy dashboard is showing 16ms). This can be a significant delay for some time-sensitive/performance-sensitive use cases.
Copy link
Contributor

fabioberger left a comment

Can we please:

  • Increase the default 24hr limit env var to 200k?
  • Add CHANGELOG entry describing this change in how 24hr limiter works.
ethereum/ratelimit/rate_limiter.go Outdated Show resolved Hide resolved
ethereum/ratelimit/rate_limiter_test.go Outdated Show resolved Hide resolved
@@ -247,7 +203,7 @@ func TestScenario4(t *testing.T) {

// If we are not properly converting times to UTC, instantiation will fail with err
// `Wait(n=450000) exceeds limiter's burst 300000`

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 17, 2019

Contributor
Suggested change
// `Wait(n=450000) exceeds limiter's burst 300000`
// `Wait(n=150000) exceeds limiter's burst 100000`
albrow and others added 10 commits Dec 12, 2019
Co-Authored-By: Fabio B <me@fabioberger.com>
Co-Authored-By: Fabio B <me@fabioberger.com>
@albrow albrow force-pushed the fix/simplify-ethereum-rpc-rate-limiting branch from 03b6797 to 83f2bad Dec 18, 2019
@albrow albrow requested a review from fabioberger Dec 18, 2019
@albrow albrow merged commit bf0afc8 into development Dec 19, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/drone/push Build is passing
Details
@albrow albrow deleted the fix/simplify-ethereum-rpc-rate-limiting branch Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.