snarkOS icon indicating copy to clipboard operation
snarkOS copied to clipboard

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

Open fulltimemike opened this issue 1 year ago • 0 comments

🐛 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 avatar Sep 23 '24 22:09 fulltimemike