Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

enha: add configurable max request size for JSON-RPC #2043

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Mar 10, 2025

PR Type

Enhancement


Description

  • Add configurable max request size for external RPC

  • Implement in RPC downloader, importer, and blockchain client

  • Update integration tests with new parameter

  • Modify deployment script to include new configuration


Changes walkthrough 📝

Relevant files
Enhancement
rpc_downloader.rs
Update RPC downloader with max request size parameter       

src/bin/rpc_downloader.rs

  • Add external_rpc_max_request_size_bytes parameter to
    BlockchainClient::new_http call
  • +1/-1     
    importer_config.rs
    Implement max request size in ImporterConfig                         

    src/eth/follower/importer/importer_config.rs

  • Add external_rpc_max_request_size_bytes field to ImporterConfig struct
  • Update init method to pass max request size to
    BlockchainClient::new_http_ws
  • +17/-1   
    rpc_server.rs
    Update RPC server to handle max request size parameter     

    src/eth/rpc/rpc_server.rs

  • Add parsing for external_rpc_max_request_size_bytes in
    stratus_init_importer function
  • Include new parameter in ImporterConfig initialization
  • +8/-1     
    blockchain_client.rs
    Implement max request size in BlockchainClient                     

    src/infra/blockchain_client/blockchain_client.rs

  • Add max_request_size_bytes field to BlockchainClient struct
  • Update new_http, new_http_ws, and build_http_client methods to include
    max request size
  • Set max request size in HttpClientBuilder
  • +13/-6   
    Configuration changes
    config.rs
    Add max request size configuration to RpcDownloaderConfig

    src/config.rs

  • Add external_rpc_max_request_size_bytes field to RpcDownloaderConfig
    struct
  • Set default value to 10485760 bytes (10MB)
  • +8/-0     
    deploy.py
    Update deployment script with max request size parameter 

    utils/deploy/deploy.py

  • Add external_rpc_max_request_size_bytes to Config class with default
    value of "10485760"
  • Include new parameter in change_to_follower_params list
  • +2/-1     
    Tests
    leader-follower-change.test.ts
    Update leader-follower change tests with max request size

    e2e/cloudwalk-contracts/integration/test/leader-follower-change.test.ts

  • Add max request size parameter (10485760) to stratus_changeToFollower
    calls in tests
  • +4/-0     
    leader-follower-importer.test.ts
    Update leader-follower importer tests with max request size

    e2e/cloudwalk-contracts/integration/test/leader-follower-importer.test.ts

  • Add max request size parameter (10485760) to stratus_initImporter
    calls in tests
  • +4/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • …ions
    
    Add a new configuration parameter `external_rpc_max_request_size_bytes` to control the maximum request size for external RPC connections. This allows more granular control over client resource limits and prevents potential large request-related issues across multiple components including RPC downloader, importer, and blockchain client.
    Copy link

    github-actions bot commented Mar 10, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit f77c361)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Unused Field

    The max_request_size_bytes field in the BlockchainClient struct is marked with #[allow(dead_code)]. This suggests that the field is not being used anywhere in the code. Consider removing this field if it's truly unused or implement its functionality if it's intended to be used.

    #[allow(dead_code)]
    max_request_size_bytes: u32,
    Error Logging

    The error logging in the stratus_init_importer function doesn't follow the guideline of logging the original error in a field called 'reason'. Consider updating the error logging to include the original error as a 'reason' field.

        tracing::error!(reason = ?e, "failed to parse external_rpc_timeout");
        ImporterError::ConfigParseError
    })?;
    
    let sync_interval = parse_duration(&raw_sync_interval).map_err(|e| {
        tracing::error!(reason = ?e, "failed to parse sync_interval");
        ImporterError::ConfigParseError
    })?;
    
    let external_rpc_max_request_size_bytes = raw_external_rpc_max_request_size_bytes.parse::<u32>().map_err(|e| {
        tracing::error!(reason = ?e, "failed to parse external_rpc_max_request_size_bytes");
        ImporterError::ConfigParseError
    })?;

    Copy link

    github-actions bot commented Mar 10, 2025

    PR Code Suggestions ✨

    Latest suggestions up to f77c361
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add logging for max request size

    Add logging for the max_request_size_bytes parameter in the build_http_client
    function to ensure it's correctly applied and to aid in debugging potential issues
    related to request size limits.

    src/infra/blockchain_client/blockchain_client.rs [80-90]

     fn build_http_client(url: &str, timeout: Duration, max_request_size_bytes: u32) -> anyhow::Result<HttpClient> {
    -    tracing::info!(%url, timeout = %timeout.to_string_ext(), "creating blockchain http client");
    +    tracing::info!(%url, timeout = %timeout.to_string_ext(), max_request_size = %max_request_size_bytes, "creating blockchain http client");
         match HttpClientBuilder::default()
             .request_timeout(timeout)
             .max_request_size(max_request_size_bytes)
             .build(url)
         {
             Ok(http) => {
    -            tracing::info!(%url, timeout = %timeout.to_string_ext(), "created blockchain http client");
    +            tracing::info!(%url, timeout = %timeout.to_string_ext(), max_request_size = %max_request_size_bytes, "created blockchain http client");
                 Ok(http)
             }
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion improves the logging by adding the max_request_size_bytes parameter, which enhances debugging capabilities and provides more comprehensive information about the HTTP client configuration.

    Medium

    Previous suggestions

    Suggestions up to commit 18aa4db
    CategorySuggestion                                                                                                                                    Impact
    General
    Use idiomatic Rust boolean negation

    Replace the not function with the ! operator for boolean negation, which is more
    idiomatic in Rust.

    src/bin/rpc_downloader.rs [44-46]

    -if not(config.external_rpc.contains("app=") || config.external_rpc.contains("/app/")) {
    +if !(config.external_rpc.contains("app=") || config.external_rpc.contains("/app/")) {
         tracing::warn!(url = config.external_rpc, "url isn't identified with '?app=NAME' query parameter");
     }
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves code readability and adheres to Rust idioms by replacing the 'not' function with the '!' operator, which is the standard way to negate boolean expressions in Rust.

    Medium

    gabriel-aranha-cw and others added 2 commits March 10, 2025 17:52
    Add max request size parameter (10485760 bytes) to leader-follower change and importer integration tests to align with new RPC configuration
    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review March 11, 2025 14:29
    Copy link

    Persistent review updated to latest commit f77c361

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Merge on Hold. Need to deploy new version of stratus-updater first.

    @gabriel-aranha-cw gabriel-aranha-cw merged commit be6bfc8 into main Mar 11, 2025
    41 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the enha-expose-request-size-limit branch March 11, 2025 20:42
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-3204714532

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    Leader Stats:
    RPS Stats: Max: 2014.00, Min: 1358.00, Avg: 1857.87, StdDev: 72.34
    TPS Stats: Max: 2002.00, Min: 1656.00, Avg: 1803.38, StdDev: 87.79

    Follower Stats:
    Imported Blocks/s: Max: 2.00, Min: 1.00, Avg: 1.64, StdDev: 0.48
    Imported Transactions/s: Max: 3950.00, Min: 565.00, Avg: 3031.23, StdDev: 903.80

    Plots:

    # for free to join this conversation on GitHub. Already have an account? # to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants