Skip to content

enha: use machine name as client identifier - #2632

Open
carneiro-cw wants to merge 2 commits into
mainfrom
stratus_client
Open

enha: use machine name as client identifier#2632
carneiro-cw wants to merge 2 commits into
mainfrom
stratus_client

Conversation

@carneiro-cw

@carneiro-cw carneiro-cw commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

PR Type

Enhancement


Description

  • Include machine name in x-client header

  • Simplify forward_to_leader call signature

  • Update RPC clients to set headers

  • Add hostname dependency


Diagram Walkthrough

flowchart LR
  A["consensus.forward_to_leader"]
  B["BlockchainClient.send_raw_transaction_to_leader"]
  C["client_headers"]
  D["HttpClientBuilder/WsClientBuilder set_headers"]
  A -- "calls" --> B
  B -- "invokes" --> C
  C -- "attaches x-client header" --> D
Loading

File Walkthrough

Relevant files
Enhancement
consensus.rs
Simplify forward_to_leader signature                                         

src/eth/follower/consensus.rs

  • Removed rpc_client parameter from forward_to_leader
  • Updated tracing to drop rpc_client logging
  • Documented x-client header behavior
+6/-4     
blockchain_client.rs
Add client headers for RPC requests                                           

src/eth/rpc/blockchain_client/blockchain_client.rs

  • Added client_headers and machine_name functions
  • Configured HTTP and WS clients to set x-client header
  • Removed rpc_client argument from transaction send
+38/-5   
server.rs
Update server RPC forwarding call                                               

src/eth/rpc/server.rs

  • Updated RPC handler to call new forward_to_leader signature
  • Removed obsolete rpc_client argument
+1/-1     
Dependencies
Cargo.toml
Add hostname dependency                                                                   

Cargo.toml

  • Added hostname crate dependency
+1/-0     

@carneiro-cw
carneiro-cw requested a review from a team as a code owner August 25, 2026 16:16

@cloudwalk-review-agent cloudwalk-review-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Looks good overall. This change consistently migrates follower->leader client identification from RPC params to transport headers (x-client) across HTTP and WS client builders, and updates call sites/signatures accordingly.

I did not find concrete correctness, security, or deploy-safety regressions in the provided diff. The parameter removal for eth_sendRawTransaction forwarding is internally consistent with the new header-based attribution model.

@cloudwalk-review-agent cloudwalk-review-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simplification overall—moving client identity to transport headers reduces API coupling and removes extra RPC params cleanly.

I found one blocking correctness/API-compat issue:

  1. Blocking: changed JSON-RPC params for eth_sendRawTransaction may break upstream leader compatibility
    In BlockchainClient::send_raw_transaction_to_leader, request changed from [tx, rpc_client] to [tx]. Unless the leader-side method now accepts the standard single-param form (or supports both), follower->leader forwarding can fail at runtime with invalid params.

    • Impact: followers may be unable to forward transactions, causing tx submission failures in follower mode.
    • Where: src/eth/rpc/blockchain_client/blockchain_client.rs (method send_raw_transaction_to_leader)
    • Suggested fix: ensure server-side eth_sendRawTransaction handler accepts header-based attribution without requiring 2nd param, ideally keeping backward compatibility for both arities during rollout; add an integration test for follower forwarding against leader endpoint.

Non-blocking note: machine_name() doc says fallback to "unknown" but code uses "stratus"—just align comment/string to avoid confusion.

@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Documentation Mismatch

The machine_name doc comment says it falls back to "unknown" on failure, but the code uses "stratus". This inconsistency could confuse readers or lead to incorrect assumptions about the header value.

fn machine_name() -> String {
    match hostname::get() {
        Ok(name) => name.to_string_lossy().into_owned(),
        Err(e) => {
            tracing::warn!(reason = ?e, "failed to get machine name, using \"stratus\"");
            "stratus".to_string()
        }
Silent Header Failure

If HttpHeaderValue::from_str(&machine_name) fails, the header insertion is skipped without any warning. It would be helpful to log a warning or error so that malformed machine names aren’t silently dropped.

fn client_headers() -> HttpHeaderMap {
    let machine_name = machine_name();
    let mut headers = HttpHeaderMap::new();
    if let Ok(value) = HttpHeaderValue::from_str(&machine_name) {
        headers.insert("x-client", value);
    }
    headers
}

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use consistent fallback name

The fallback value in the doc comment is “unknown” but the code uses “stratus”.
Update the returned string and warning message to match the documented behavior.

src/eth/rpc/blockchain_client/blockchain_client.rs [66-68]

 Err(e) => {
-    tracing::warn!(reason = ?e, "failed to get machine name, using \"stratus\"");
-    "stratus".to_string()
+    tracing::warn!(reason = ?e, "failed to get machine name, using \"unknown\"");
+    "unknown".to_string()
 }
Suggestion importance[1-10]: 6

__

Why: The doc comment says the fallback is "unknown" but the code warns about and returns "stratus", creating an inconsistency between machine_name's behavior and its documentation.

Low
Warn on header parse failure

If header value parsing fails, it is dropped silently. Adding a warning on failure
will aid debugging when the x-client header isn’t set.

src/eth/rpc/blockchain_client/blockchain_client.rs [56-58]

 if let Ok(value) = HttpHeaderValue::from_str(&machine_name) {
     headers.insert("x-client", value);
+} else {
+    tracing::warn!("failed to parse machine name for x-client header: {}", machine_name);
 }
Suggestion importance[1-10]: 5

__

Why: Adding an else with tracing::warn! for HttpHeaderValue::from_str(&machine_name) failures improves visibility when the x-client header isn't set, aiding debugging without affecting core functionality.

Low

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1e193f745

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +56 to +58
if let Ok(value) = HttpHeaderValue::from_str(&machine_name) {
headers.insert("x-client", value);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve a fallback for header-unsafe hostnames

When hostname::get() succeeds but returns a hostname containing non-ASCII or control characters, HttpHeaderValue::from_str fails and this branch silently omits x-client. The leader's parse_client_app then attributes these requests to Unknown, defeating the new identification behavior; use the documented fallback value or return an error when the hostname cannot be encoded as a header.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant