-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_http_server: add configurable max_request_body_size as 5mb #12366
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
apollo_http_server: add configurable max_request_body_size as 5mb #12366
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
47b53af to
4f57e64
Compare
4f57e64 to
2a03207
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
crates/apollo_deployments/resources/app_configs/http_server_config.json
Outdated
Show resolved
Hide resolved
meship-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meship-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @TzahiTaub).
crates/apollo_http_server_config/src/config.rs line 15 at r2 (raw file):
// The value is chosen to be much larger than the transaction size limit as enforced by the Starknet // protocol. const DEFAULT_MAX_REQUEST_BODY_SIZE: usize = 5 * 1024 * 1024; // 5MB
This is not the constraint, right? As for now, transactions with proof can reach 6MB; we aim to lower it, but for the test env we need a larger margin.
Code quote:
const DEFAULT_MAX_REQUEST_BODY_SIZE: usize = 5 * 1024 * 1024; // 5MB
TzahiTaub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TzahiTaub made 1 comment.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @eitanm-starkware, @meship-starkware, and @ob1337).
crates/apollo_http_server_config/src/config.rs line 15 at r2 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
This is not the constraint, right? As for now, transactions with proof can reach 6MB; we aim to lower it, but for the test env we need a larger margin.
57eaeb6 to
8efb25f
Compare
TzahiTaub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TzahiTaub reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware).
a discussion (no related file):
Please add a check (or TODO here) when polling the dynamic config, that max_sierra_program_size > max_request_body_size and log a warning if it isn't.
8efb25f to
3bac13d
Compare
ArniStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArniStarkware made 1 comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @TzahiTaub).
a discussion (no related file):
Previously, TzahiTaub (Tzahi) wrote…
Please add a check (or TODO here) when polling the dynamic config, that
max_sierra_program_size > max_request_body_sizeand log awarningif it isn't.
Done, Addressed in #12438
TzahiTaub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TzahiTaub reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware).
4188396

Note
Low Risk
Small, localized config + routing change that only affects request size handling; main risk is inadvertently rejecting/allowing larger requests due to misconfiguration.
Overview
Adds a new static HTTP server config
max_request_body_size(default 5MB) and exposes it through config dumping/schema and deployment app config templates.Updates the Axum router so POST endpoints (
/gateway/add_transactionand/gateway/add_rpc_transaction) enforce the configured request body limit viaDefaultBodyLimit.Written by Cursor Bugbot for commit 3bac13d. This will update automatically on new commits. Configure here.