[MegaRedHand]

Second step of MEV-Boost builder support: an HTTP client for the builder API, and the builder signing domain. Stacked on #694. Nothing constructs the client yet; the next PR adds --builder and wires it into produceBlockV3, register_validator and blinded_blocks.

What it adds

Piece	Where
BuilderClient: get_header, register_validators, submit_blinded_block; VersionedBid; BuilderError	rpc/src/beacon/builder_client/ (new), exported from ethlambda_rpc
compute_builder_domain(genesis_fork_version), with a mainnet known-answer test	types/src/beacon/signing.rs
lean_beacon_builder_request_seconds{call, outcome}	rpc/src/metrics.rs, docs/metrics.md
reqwest (the workspace pin, rustls) and thiserror as direct dependencies	rpc/Cargo.toml
A #[cfg(test)] axum mock relay, reused by the next PR's endpoint tests	builder_client/mock_relay.rs

Encoding, call by call

Each call picks its encoding from how MEV-Boost actually handles it:

Call	Encoding	Why
getHeader	Asks SSZ-first and decodes by the answer's own Content-Type. A 406 fails that call with no retry; later calls ask for JSON	Relays often answer JSON without refusing SSZ. A retry would restart the clock the proposer is waiting on
registerValidator	Always JSON	MEV-Boost forwards the body unchanged to every relay and answers 200 if any one accepts, so SSZ would be lost silently on relays without it
submitBlindedBlockV2	SSZ first; after its own 415, the same block again in JSON, and JSON from then on	MEV-Boost converts SSZ per relay. A relay used directly answers 415 at once, so the retry costs nothing

Strict choices and bounds

• An SSZ bid without Eth-Consensus-Version is an error, since builder-specs requires the header on SSZ answers.
• Only electra and fulu bids are accepted (same container). Any other fork is a separate UnsupportedFork error, so the caller can count it.
• submitBlindedBlockV2 succeeds on 202 only.
• Timeouts:
  • getHeader uses the configured header timeout, which is also sent as X-Timeout-Ms alongside Date-Milliseconds.
  • Registration has 10 s.
  • Submission has 5 s, which is at least MEV-Boost's 4 s.
• Answers are capped at 8 MiB and kept error text at 1 KiB.
• Transport errors drop the URL, so credentials in it cannot reach a log.
• Only http and https builder URLs are accepted, which catches a missing scheme at startup.

Tests

The client has 27 tests against the mock relay. The mock relay:

• serves signed bids;
• verifies registration signatures under the builder domain;
• records submissions;
• has knobs for the ways real builders differ: refusing SSZ, ignoring Accept, answering slowly, leaving out headers, or giving a chosen status.

make lint passes, as do the ethlambda-rpc and ethlambda-types lib tests.

[github-actions]

🤖 Claude Code Review

Review of PR 695: builder API client and builder signing domain

The change is well scoped, and nothing in it constructs the client yet, so it can't affect a running node. I read the diff only; I did not build it or run the tests.

Looks good

• compute_builder_domain reuses compute_domain with a zero validators root and has a mainnet known-answer test. Registrations and bids are signed under the right domain.
• The response cap in read_body is enforced per chunk, so a large body is rejected before it is buffered. Error text is truncated at a char boundary, without_url() keeps credentials out of logs, and the timeout covers the body read.
• Decoding follows the response's own Content-Type. An SSZ bid with no Eth-Consensus-Version is rejected, and bid_fork accepts only Electra and Fulu, which share a container.
• Metric labels have bounded cardinality. The mock relay checks registration signatures and holds no mutex across an .await.

Points to consider

1. Bid signature is not verified here (mod.rs, get_header and decode_bid). The client returns an unverified SignedBuilderBid. The next PR must check the signature under compute_builder_domain, the pubkey, the parent hash, and the value and gas-limit bounds before a bid wins. Otherwise a malicious relay or MEV-Boost could get a payload header signed by the proposer. A doc note on VersionedBid saying it is unverified would stop a caller assuming otherwise.
2. One json_only flag covers both getHeader and submit (mod.rs:596-598, fall_back_to_json). A 406 on getHeader also switches submissions to JSON, and a 415 on submit switches getHeader to JSON. This is safe, since JSON works everywhere, but it gives up SSZ efficiency on a call that didn't refuse it. Separate flags per call, or a comment saying the sharing is deliberate, would be clearer. This also stands in for the doc's "per URL".
3. submit_blinded_block can take up to 10 s (mod.rs:760-781). After a 415 it sends the block again with a fresh SUBMIT_TIMEOUT. A 415 normally arrives at once, so this is acceptable, but a slow 415 doubles the proposer's wait. A single deadline shared across both attempts would bound it.
4. Redirects are followed. reqwest::Client::builder() follows up to 10 redirects by default. A builder or proxy that redirects a getHeader could spend the proposer's time budget on another host, and a redirected POST can lose or change its body. Setting redirect(Policy::none()) is the safer choice for this API.
5. from_status hides body-read errors (error.rs:147). read_body(...).unwrap_or_default() throws away TooLarge and timeout errors, so a 400 whose body failed to read shows as BadRequest(""). This is minor.
6. submit_blinded_block doesn't check fork (mod.rs:749-753). It sends whatever ForkName it is given as Eth-Consensus-Version, while get_header restricts bids to Electra and Fulu. A debug_assert! or an early UnsupportedFork would keep the two consistent.

No blocking issues. Point 1 is the most important to settle in the follow-up PR.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

• Overall this looks solid: the client is careful about body-size caps, avoids leaking URLs from reqwest errors, and the SSZ→JSON fallback behavior is well tested.

• crates/net/rpc/src/beacon/builder_client/mod.rs:307

  • SignedBuilderBid::from_ssz_bytes(&body) decodes the SSZ payload based only on the Eth-Consensus-Version header, but there is no cryptographic or semantic validation that the bid’s contents are actually valid for that fork or signed under the expected builder domain.
  • If validation happens elsewhere, fine; if not, this is a consensus-critical gap because a malicious relay can send a syntactically valid bid with an invalid signature or malformed contents and push bad data deeper into block production.
  • Suggestion: either validate the bid signature here before returning VersionedBid, or add an explicit comment/reference showing where that validation is guaranteed downstream.

• crates/net/rpc/src/beacon/builder_client/mod.rs:248

  • The client retries submitBlindedBlockV2 in JSON after any 415 Unsupported Media Type from the first SSZ submission. That is convenient for MEV-Boost compatibility, but it also treats all 415s as “this endpoint doesn’t support SSZ”.
  • A misconfigured or malicious intermediary could return 415 for reasons unrelated to permanent SSZ support, causing the client to globally flip json_only for that URL and remain there. That is a persistent downgrade on a shared Arc<AtomicBool>.
  • Consider limiting the downgrade to cases where the first request was definitely SSZ and the response body or server identity matches the expected MEV-Boost behavior, or at least make the downgrade temporary/resettable.

• crates/net/rpc/src/beacon/builder_client/mod.rs:277

  • fall_back_to_json() logs every downgrade with info!, but it does not include the builder endpoint identity. In deployments with multiple builder URLs, this makes diagnosis hard.
  • Consider logging a sanitized host or an explicit builder identifier while still avoiding credential leakage.

• crates/net/rpc/src/beacon/builder_client/mod.rs:282

  • url() uses format!("{}{path}", self.base_url). Since new() only validates the base URL once and then stores the original string, unusual-but-parseable inputs like embedded path segments or query strings can produce surprising request URLs when concatenated.
  • Example risk: a configured base like https://relay.example/api?x=1 becomes https://relay.example/api?x=1/eth/v1/..., which is malformed semantically but won’t be caught after construction.
  • Prefer storing a parsed reqwest::Url and using Url::join or path mutation instead.

• crates/net/rpc/src/beacon/builder_client/error.rs:65

  • from_status() does read_body(response, deadline).await.unwrap_or_default(), which silently discards TooLarge / transport errors while reading an error body and then collapses them into an empty-message Status or BadRequest.
  • That loses useful signal and can hide a builder returning oversized error pages or disconnecting mid-error-response.
  • Suggestion: preserve the read failure, e.g. return TooLarge/Transport directly, or at minimum wrap it into the status error text.

• crates/net/rpc/src/beacon/builder_client/mod.rs:313

  • The JSON decode path parses the same body twice. The comment explains why, and this is acceptable at builder-bid sizes, but it is still avoidable extra CPU and allocation on the proposer critical path.
  • A small struct like { version: String, data: SignedBuilderBid } would decode in one pass and keep the same readability.

• crates/common/types/src/beacon/signing.rs:79

  • compute_builder_domain() looks correct and the mainnet-vector test is a good addition. Nice to see the builder domain separated explicitly from in-protocol domains.

• No consensus-layer regressions are visible in fork choice, attestation handling, justification/finalization, state transition, XMSS, or SSZ logic from this diff; this PR is scoped to builder RPC/client behavior and metrics.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds a builder API client for MEV-Boost/relay communication. Let me analyze the code systematically.

Overall Assessment

This is a well-structured PR adding builder-specs client functionality with good test coverage. However, I've identified several issues ranging from minor correctness problems to potential security and reliability concerns.

---

Security & Correctness Issues

1. Race condition in header() handler — mock_relay.rs:199-201

let delay = state.lock().unwrap().delay;
tokio::time::sleep(delay).await;

let mut state = state.lock().unwrap();  // re-acquires lock after await

Problem: The lock is dropped before .await, then re-acquired. Between these points, another task could modify delay, raw_answer, or other state, causing the observed delay to differ from what was intended, or the response to use stale configuration.

Fix: Keep the lock guard across the await by using a scoped approach, or clone all needed state before the await:

let (delay, raw_answer, bid, refuses_ssz, ignores_accept, omits_consensus_version) = {
    let state = state.lock().unwrap();
    (state.delay, state.raw_answer.clone(), /* ... clone needed fields */)
};
tokio::time::sleep(delay).await;
// use cloned values

---

2. Missing eth-consensus-version header on JSON responses in mock_relay.rs

The mock relay never sets eth-consensus-version on JSON responses (line 261-263), which is correct per MEV-Boost behavior. However, the client's decode_bid function (mod.rs:298-321) relies on this header for SSZ but not JSON. This asymmetry is properly handled, but the comment in mock_relay.rs:78-79 says "JSON answers never carry it, as MEV-Boost's do not" — this should be verified against actual MEV-Boost behavior, as some relays may differ.

---

3. Potential panic in unix_millis() — mod.rs:399-402

fn unix_millis() -> u128 {
    SystemTime::now()
        .duration_since(UNIX_EPOCH)
        .map_or(0, |since| since.as_millis())
}

Problem: SystemTime::now() can return times before UNIX_EPOCH (clock skew), silently returning 0. This is used for the date-milliseconds header. While unlikely, a 0 value could confuse builders doing request freshness checks.

Fix: Consider using unwrap_or_else with a monotonic fallback, or at minimum document this behavior. For builder API timing, Instant would be more appropriate for relative timing, but this is sent to the remote server so absolute time is needed.

---

4. SSZ content type constant missing — mod.rs references crate::SSZ_CONTENT_TYPE

The code references crate::SSZ_CONTENT_TYPE (lines 139, 290, 332) but this constant is not defined in the visible diff. Verify this exists in the crate root; if not, this will fail to compile.

---

Error Handling & Reliability

5. Incomplete timeout handling in from_reqwest — error.rs:52-59

pub(super) fn from_reqwest(err: reqwest::Error, deadline: Duration) -> Self {
    let err = err.without_url();
    if err.is_timeout() {
        Self::Timeout(deadline)
    } else {
        Self::Transport(err.to_string())
    }
}

Problem: reqwest distinguishes between request timeout (deadline exceeded) and connect timeout. A connect timeout during TCP handshake should arguably be Transport, but is_timeout() returns true for both. This is acceptable but the Timeout variant's documentation says "No complete answer within the call's deadline" — a connect timeout is arguably different.

More critically: err.is_timeout() also returns true for response body read timeouts, which read_body handles separately with its own deadline logic. This could lead to double-counting or confusing metrics.

---

6. read_body doesn't limit individual chunk size — mod.rs:324-340

while let Some(chunk) = response.chunk().await /* ... */ {
    if body.len() + chunk.len() > MAX_RESPONSE_BYTES {
        return Err(BuilderError::TooLarge { limit: MAX_RESPONSE_BYTES });
    }
    body.extend_from_slice(&chunk);
}

Problem: No limit on individual chunk.len(). A malicious server could send a single 8GB chunk, causing memory allocation failure before the length check. reqwest typically chunks at 16KB, but this isn't guaranteed.

Fix: Add a per-chunk limit:

if chunk.len() > MAX_RESPONSE_BYTES || body.len() + chunk.len() > MAX_RESPONSE_BYTES {
    return Err(BuilderError::TooLarge { limit: MAX_RESPONSE_BYTES });
}

---

7. Silent truncation in truncate_error_text — mod.rs:343-353

pub(super) fn truncate_error_text(mut text: String) -> String {
    if text.len() > MAX_ERROR_TEXT {
        let mut end = MAX_ERROR_TEXT;
        while !text.is_char_boundary(end) {
            end -= 1;
        }
        text.truncate(end);
    }
    text
}

Problem: If MAX_ERROR_TEXT (1024) falls in the middle of a multi-byte UTF-8 sequence, this walks backward to find a char boundary. For pathological inputs (e.g., 1024-byte string of 4-byte UTF-8 chars where no boundary exists near 1024), this could walk to 0. More realistically, if text is valid UTF-8, is_char_boundary will find a boundary quickly, but this isn't guaranteed for invalid UTF-8 from String::from_utf8_lossy.

Actually — String::from_utf8_lossy produces valid UTF-8, so this is fine. But the function should document that it may truncate in the middle of a logical message.

---

API Design & Maintainability

8. Inconsistent pub(crate) vs pub(super) visibility — error.rs

• from_reqwest is pub(super) (line 51)
• from_status is pub(super) (line 62)
• label is pub(crate) (line 83)

Since error.rs is a submodule of builder_client, pub(super) restricts to builder_client module, while label is accessible to all of crate::beacon. This seems intentional but inconsistent — label is used in mod.rs for metrics, which is same module. Consider making all pub(super) unless external use is needed.

---

9. Missing builder domain constant documentation typo — signing.rs:79-80

/// builder domain; see [`DOMAIN_APPLICATION_BUILDER`].

"builder domain" should be "in-protocol builder domain" for clarity, matching the contrast with "builder-specs' own messages" earlier in the comment. Minor typo: "gloas's" should be "gossamer's" or similar — unclear what "gloas" refers to.

---

10. Test helper bid_from creates invalid bid pubkey — tests.rs:35-48

fn bid_from(relay: &MockRelay, fork: ForkName) -> VersionedBid {
    let message = BuilderBid {
        // ...
        pubkey: BlsPubkey([0; 48]),  // ZERO pubkey!
    };
    VersionedBid {
        fork,
        bid: relay.sign_bid(message),  // overwrites pubkey with relay's key
    }
}

The pubkey: BlsPubkey([0; 48]) is immediately overwritten by relay.sign_bid(). This is confusing — the zero pubkey is never used. Consider making sign_bid take BuilderBid by value and document that it sets pubkey, or use a builder pattern.

---

11. No validation of BuilderClient URL path traversal — mod.rs:158-160

fn url(&self, path: &str) -> String {
    format!("{}{path}", self.base_url)
}

If path starts with /, this works (base_url has trailing slash stripped). But if a future caller passes a path without leading /, the URL is malformed. Consider assert!(path.starts_with('/')) or using reqwest::Url::join.

---

Performance & Resource Management

12. Clone-heavy design in MockRelay state access

Every accessor clones Vec<T> from MutexGuard:

pub(crate) fn blinded_blocks(&self) -> Vec<SignedBlindedBeaconBlock> {
    self.state.lock().unwrap().blinded_blocks.clone()
}

This is test code so acceptable, but for large vectors this is O(n). Consider returning slices or using Arc<[T]> if tests grow.

---

13. jemalloc_pprof dependency added but unused in visible diff

Cargo.toml adds reqwest and thiserror, but jemalloc_pprof was already present. Verify reqwest features are minimal — default features include cookies, gzip, brotli, etc. which may be unnecessary. Consider:

reqwest = { workspace = true, default-features = false, features = ["json"] }

Or whatever features are actually needed (likely json, rustls-tls or native-tls).

---

Consensus-Specific Observations

14. Fork name parsing is case-insensitive but only lowercases — mod.rs:372-378

fn bid_fork(version: &str) -> Result<ForkName, BuilderError> {
    match ForkName::parse(version) {
        Some(fork @ (ForkName::Electra | ForkName::Fulu)) => Ok(fork),
        _ => Err(BuilderError::UnsupportedFork(version.to_owned())),
    }
}

The to_ascii_lowercase() call at line 301 means this receives lowercase input. The test an_uppercase_version_is_read verifies this works for JSON. However, for SSZ, the header value is lowercased at line 300. This is correct but the ForkName::parse should document its case sensitivity.

---

15. Electra/Fulu bid shape assumption — mod.rs:372-378

Some(fork @ (ForkName::Electra | ForkName::Fulu)) => Ok(fork),

This hardcodes that only Electra and Fulu use the SignedBuilderBid container shape. This is correct per current specs but will need updating for future forks. Add a prominent comment linking to the builder-specs version this implements.

---

Minor Issues

16. Typo in comment — mock_relay.rs:12

/// `answer_headers_with` and
/// `answer_submissions_with` to answer a call with exactly what a test needs.

Missing verb: should be "has answer_headers_with and answer_submissions_with to answer..."

17. Inconsistent use of crate::SSZ_CONTENT_TYPE vs literal "application/octet-stream"

The ACCEPT_SSZ_FIRST constant uses "application/octet-stream;q=1.0,application/json;q=0.9" but SSZ_CONTENT_TYPE (referenced elsewhere) may differ. Ensure these stay synchronized.

---

Positive Observations

1. Good test coverage: Comprehensive tests for success paths, error paths, encoding fallback, timeout behavior, and malformed responses.
2. Proper SSZ/JSON fallback logic: The 406→JSON fallback with atomic flag is correct and avoids retry storms.
3. URL credential stripping: without_url() and explicit scheme validation show security awareness.
4. Metrics integration: Proper histogram labels for observability.
5. Builder domain computation: Correctly uses genesis fork version with zero validators root, matching builder-specs.

---

Summary Table

Priority	Item	File	Line	Issue
High	1	mock_relay.rs	199-201	Race condition in header() handler
High	6	mod.rs	324-340	Unbounded chunk size in read_body
Medium	5	error.rs	52-59	Timeout classification ambiguity
Medium	3	mod.rs	399-402	unix_millis() clock skew handling
Medium	13	Cargo.toml	—	reqwest feature bloat
Low	9	signing.rs	79-80	Documentation typo ("gloas's")
Low	10	tests.rs	35-48	Confusing zero pubkey in test helper
Low	11	mod.rs	158-160	URL path construction fragile
Low	16	mock_relay.rs	12	Comment grammar

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt