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

[Bug] Rest request per second is actually burst per second, and returns 200s instead of errors #3402

Closed
fulltimemike opened this issue Sep 23, 2024 · 0 comments · Fixed by #3405
Labels
bug Incorrect or unexpected behavior

Comments

@fulltimemike
Copy link
Collaborator

🐛 Bug Report

  1. The GovernorConfigBuilder is configured to only replenish 1 element of the quota every second per ip address, regardless of the rest_rps variable passed in. The rest_rps variable is being used to set the burst_size per ip address, but assuming the burst_size is exhausted, the effective rate limit of a client node is 1 request per second per ip. My suggested fix would be to use .per_nanosecond((1_000_000_000 / rest_rps) as u64) in place of .per_second(1), as this will replenish the rest_rps fully every second. Bug location: https://github.com/AleoNet/snarkOS/blob/mainnet-staging/node/rest/src/lib.rs#L114
  2. 429s (too many requests) (and any other errors from the rate-limiting library) end up being swallowed and returned as 200s. As a patch, I'm going to use this in place of the current error handler:
                // Properly return a 429 Too Many Requests error
                let error_message = error.to_string();
                let mut response = Response::new(error_message.clone().into());
                *response.status_mut() = StatusCode::INTERNAL_SERVER_ERROR;
                if error_message.contains("Too Many Requests") {
                  *response.status_mut() = StatusCode::TOO_MANY_REQUESTS;
                }
                response
            })

Location: https://github.com/AleoNet/snarkOS/blob/mainnet-staging/node/rest/src/lib.rs#L116

Steps to Reproduce

Run a local devnet with a low rps, like 2. Hit an endpoint twice, and observe you can only hit the rest endpoint once per second thereafter.

Expected Behavior

Errors should contain error codes, not 200s. Rest rps should be the actual requests per second and not just the burst metric.

Your Environment

  • snarkOS Version
  • v2.2.7
  • Rust Version
  • 1.81.0
  • Computer OS
  • observed locally on macOS, as well as on the demox client running on ubuntu
@fulltimemike fulltimemike added the bug Incorrect or unexpected behavior label Sep 23, 2024
@fulltimemike fulltimemike changed the title [Bug] [Bug] Rest reqeust per second is actually burst per second, and returns 200s instead of errors Sep 23, 2024
@fulltimemike fulltimemike changed the title [Bug] Rest reqeust per second is actually burst per second, and returns 200s instead of errors [Bug] Rest request per second is actually burst per second, and returns 200s instead of errors Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect or unexpected behavior
Projects
None yet
1 participant