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

Allow rate limiting clients #1420

Open
SuperFluffy opened this issue May 13, 2024 · 1 comment
Open

Allow rate limiting clients #1420

SuperFluffy opened this issue May 13, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@SuperFluffy
Copy link
Contributor

Description

I need to rate limit our tendermint-rpc HttpClient to not DOS our cometbft node. Unfortunately, tendermint_rpc::client::http::Builder and tendermint_rpc::client::http::HttpClient do not provide ways to set a tower middleware like its RateLimitLayer.

Prior art for how this could be achieved is for example by providing a set_middleware method as is done by jsonrpsee here: https://docs.rs/jsonrpsee-http-client/0.22.5/jsonrpsee_http_client/struct.HttpClientBuilder.html#method.set_http_middleware

Internally, they wrap their hyper client in the provided ServiceBuilder: https://docs.rs/jsonrpsee-http-client/0.22.5/src/jsonrpsee_http_client/transport.rs.html#254

I am going to try and work around it using the techniques described in this reqwest issue tracker here: seanmonstar/reqwest#491 (specifically using tower::ServiceBuilder::service_fn), but that requires a lot of extra boilerplate at the application level.

Definition of "done"

Being able to rate-limit outgoing requests over http. The general solution would be to allow setting arbitrary tower::Services, but rate limiting would be great.

@SuperFluffy SuperFluffy added the enhancement New feature or request label May 13, 2024
@romac
Copy link
Member

romac commented May 14, 2024

Sounds good to me, would gladly accept a PR for this :)

github-merge-queue bot pushed a commit to astriaorg/astria that referenced this issue May 27, 2024
## Summary
Limits the number of requests conductor sends to the Sequencer CometBFT
endpoint to 100 per minute.

## Background
During sync conductor can DOS Sequencer's CometBFT node by sending too
many requests for commits and validator sets. With the batching logic
introduced in #1049 there can
be dozens (or more) blocks stored in each Celestia blob, each of which
needs to be checked separately. With several blobs being fetched at once
during, this can quickly spiral into hundreds (if not thousands)
requests per minute.

Note that only calls to `/commit` and `/validators` are rate limited,
because there is currently no way to enforce this at the transport
layer, see this issue:
informalsystems/tendermint-rs#1420

However, the only other calls are to `/genesis` (once at startup), and
`/abci_info` (every block-time period, usually every 2 seconds), which
is rare enough to not need a rate limit.

## Changes
- Use a tower `RateLimit` middleware around a tendermint-rs `HttpClient`
only send up to 100 requests per minute.

## Breaking changes
- Adds an environment variable
`ASTRIA_CONDUCTOR_SEQUENCER_REQUESTS_PER_SECOND` to configure
rate-limiting of requests sent to the Sequencer CometBFT node for
verification of Sequencer block data fetched from Celestia blobs

## Testing
This needs to be observed end-to-end, potentially letting conductor run
for a very long time with only soft commits, and then turning firm
commits on.

## Related Issues
closes #1064

---------

Co-authored-by: Jordan Oroshiba <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants