snarkOS
snarkOS copied to clipboard
[Bug] Rest request per second is actually burst per second, and returns 200s instead of errors
🐛 Bug Report
- The
GovernorConfigBuilderis configured to only replenish 1 element of the quota every second per ip address, regardless of therest_rpsvariable passed in. Therest_rpsvariable is being used to set the burst_size per ip address, but assuming theburst_sizeis 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 therest_rpsfully every second. Bug location: https://github.com/AleoNet/snarkOS/blob/mainnet-staging/node/rest/src/lib.rs#L114 - 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