feat(conductor)!: rate limit sequencer cometbft requests #1068
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SuperFluffy
requested review from
Fraser999 and
joroshiba
and removed request for
noot
Fraser999
reviewed
May 14, 2024
| ); | ||
| Ok(header) | ||
| .inspect_err(|e| { | ||
| warn!( |
There was a problem hiding this comment.
This used to be info-level before. Probably better as a warn-level, but just wanted to highlight that.
| } | ||
| }); | ||
| // XXX: This number is arbitarily set to the same number os the rate-limit. Does that | ||
| // make sense? Should the number be set higher? |
There was a problem hiding this comment.
I think it probably does make sense since it should be at least the maximum number of concurrent requests the Buffer will see, although I also can't see an issue with setting it higher (within reason).
SuperFluffy
force-pushed
the
superfluffy/rate-limit-conductor
branch
from
50aa548 to
f0fecad
Compare
SuperFluffy
changed the title
|
Rebased on top of main to fetch bump the charts (should have probably just made a merge commit) |
Fraser999
approved these changes
May 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
/commitand/validatorsare rate limited, because there is currently no way to enforce this at the transport layer, see this issue: informalsystems/tendermint-rs#1420However, 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
RateLimitmiddleware around a tendermint-rsHttpClientonly send up to 100 requests per minute.Breaking changes
ASTRIA_CONDUCTOR_SEQUENCER_REQUESTS_PER_SECONDto configure rate-limiting of requests sent to the Sequencer CometBFT node for verification of Sequencer block data fetched from Celestia blobsTesting
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