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

REST rate limiting #2439

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from
Open

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Jun 14, 2023

This PR introduces a rate limiting service (with a limit of 5 reqs/s) to the REST server.

@ljedrz ljedrz requested a review from niklaslong June 14, 2023 14:18
@ljedrz ljedrz linked an issue Jun 14, 2023 that may be closed by this pull request
@howardwu
Copy link
Member

Can this be per IP?

@howardwu howardwu deleted the branch ProvableHQ:staging June 14, 2023 15:50
@howardwu howardwu closed this Jun 14, 2023
@howardwu
Copy link
Member

^This was Github. Sorry, I don't know why these "auto-closes" keep happening. In snarkVM, the branches usually retarget, rather than closing.

@niklaslong niklaslong reopened this Jun 14, 2023
niklaslong
niklaslong previously approved these changes Jun 14, 2023
@niklaslong
Copy link
Collaborator

niklaslong commented Jun 14, 2023

(pending the per IP Q)

@ljedrz
Copy link
Collaborator Author

ljedrz commented Jun 15, 2023

Can this be per IP?

There is a crate that facilitates this, but it doesn't seem like it's widely used or well-maintained; the current version is 0.0.4 and the last commit is from 6 months ago.

@howardwu
Copy link
Member

Would it be inefficient/unwise for us to implement our own?

It seems we can use an LRUMap on 100K IPs, and track each IP's last n request timestamps, where n is 5 * 60 for 5 req/s averaged over 60 seconds (to give tolerance for bursty activity, potentially from network latency or scheduler latency)

@howardwu
Copy link
Member

howardwu commented Sep 4, 2023

Args, this is once again an auto-close by Github. A unique issue to only this repo.

@howardwu howardwu reopened this Sep 4, 2023
@howardwu howardwu deleted the branch ProvableHQ:staging September 4, 2023 08:10
@howardwu howardwu closed this Sep 4, 2023
@joske
Copy link
Contributor

joske commented Sep 4, 2023

and again :-D

@howardwu howardwu reopened this Sep 4, 2023
@howardwu howardwu changed the base branch from staging to testnet3 October 15, 2023 20:35
@howardwu
Copy link
Member

@ljedrz I think we have RPS rate limiting now. Is this PR tangential?

@howardwu
Copy link
Member

#2942 says that this PR may be complementary. Can you add some color?

@ljedrz
Copy link
Collaborator Author

ljedrz commented Mar 13, 2024

The rate-limiting in this PR is not related to the source IP - it's a limit applicable to the entire REST server, to avoid a DDoS angle. However, if we are content with the limits we impose per IP, it may be redundant. We can always return to this if we have a stress test that executes many REST requests from unique source IPs, and determine that it's needed.

@howardwu
Copy link
Member

@ljedrz can you resolve the merge conflicts and set this limit to be at 100 requests per second?

For reader's context: snarkOS includes a per IP rate limiter that can be set as a CLI argument. By default, this is set to 10 requests per second (RPS) on a per IP basis. This PR is mean to limit the total requests across all IPs as an upper bound.

@ljedrz ljedrz force-pushed the rest_rate_limiting branch from 401f981 to 047cb3d Compare March 27, 2024 12:35
@ljedrz ljedrz changed the base branch from testnet3 to mainnet-staging March 27, 2024 12:35
@ljedrz
Copy link
Collaborator Author

ljedrz commented Mar 27, 2024

Cc https://github.com/AleoHQ/snarkOS/issues/3190 for that one unrelated CI failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] RPC rate limiter
4 participants