fix: increase http server limit#7314
Conversation
WalkthroughA maximum request body size of 4096 bytes is now enforced on the POST Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/minotari_node/src/http/server.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
applications/minotari_node/src/http/server.rs (1)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: ci
- GitHub Check: file licenses
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (1)
applications/minotari_node/src/http/server.rs (1)
7-7: LGTM: Import addition is correct.The import of
DefaultBodyLimitfromaxum::extractis properly added to support the body limit layer.
| .route( | ||
| "/json_rpc", | ||
| post(handler::json_rpc::handle::<B>).layer(DefaultBodyLimit::max(4096)), | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the 4096 byte limit is appropriate for JSON-RPC requests.
The technical implementation is correct, but the 4096 byte limit might be too restrictive for complex JSON-RPC requests. Consider the following concerns:
- Potential restriction: JSON-RPC requests with large parameters or batch requests may exceed 4096 bytes
- Hardcoded limit: The limit should be configurable rather than hardcoded
- PR title discrepancy: The PR title mentions "increase" but this sets a specific limit
Please verify that 4096 bytes is sufficient for expected JSON-RPC requests and consider making this configurable:
🏁 Script executed:
#!/bin/bash
# Description: Check for existing JSON-RPC request examples and configurations
# Expected: Find JSON-RPC handlers and sample requests to assess typical size
# Search for JSON-RPC handlers and related code
rg -A 10 -B 5 "json_rpc" --type rust
# Look for any existing size limits or configurations
rg -A 5 -B 5 "body.*limit|limit.*body|request.*size|size.*request" --type rust
# Check for any configuration files that might define limits
fd -e toml -e yaml -e json | head -10 | xargs grep -l "limit\|size\|json" || echo "No config files found with limit/size settings"Length of output: 57711
Make JSON-RPC body limit configurable and verify size
The .layer(DefaultBodyLimit::max(4096)) correctly applies a 4 KiB limit, but:
- There’s no existing configuration key for HTTP body limits in any TOML/YAML/JSON.
- JSON-RPC payloads (especially large transactions or batch calls) can easily exceed 4 KiB.
- The PR title suggests “increase,” yet we’re hard-coding a new fixed limit without context.
Please update applications/minotari_node/src/http/server.rs (around lines 78–81) to:
- Replace the hardcoded
4096with a value loaded from configuration (e.g.http.json_rpc_max_body_size_bytes), with a sensible default. - Add the new config key to the application’s TOML (and document it).
- Verify (or provide benchmarks/examples) that the chosen default comfortably covers expected JSON-RPC request sizes.
🤖 Prompt for AI Agents
In applications/minotari_node/src/http/server.rs around lines 78 to 81, replace
the hardcoded 4096 byte limit in DefaultBodyLimit::max with a value loaded from
a new configuration key, such as http.json_rpc_max_body_size_bytes, providing a
sensible default if the config is missing. Add this new configuration key to the
application's TOML file with documentation explaining its purpose and
recommended default size, ensuring the default is large enough to handle typical
JSON-RPC payloads including large transactions or batch calls.
Test Results (CI) 3 files 135 suites 36m 34s ⏱️ Results for commit a6a822a. |
MCozhusheck
left a comment
MCozhusheck
left a comment
There was a problem hiding this comment.
Could this limit be not enough in cases of paginated transaction list from gRPC method get_all_completed_transactions?
Also is this limit for whole message or just one chunk if response is a stream?
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
utACK
I would up more size limits form the default, and increase it from 4MiB.
| use std::sync::Arc; | ||
|
|
||
| use axum::{ | ||
| extract::DefaultBodyLimit, |
There was a problem hiding this comment.
We can also add the same increased size limit to these routes:
"/get_utxos_mined_info","/get_utxos_deleted_info""/sync_utxos_by_block""/get_utxos_by_block"
There was a problem hiding this comment.
I dont think thats needed, and thats set on the client, not server
Test Results (Integration tests)1 tests 1 ✅ 0s ⏱️ For more details on these parsing errors, see this check. Results for commit a6a822a. |
|
So this only applies to the post of the transaction to the base node, higher is not needed, this needs to fit into a block still |
3 participants
Description
fixes http limits
Summary by CodeRabbit
/json_rpcendpoint to enhance request handling.