[MegaRedHand]

Third step of MEV-Boost builder support. The beacon node can now be pointed at a builder, and it serves register_validator. Stacked on #695.

Scope: block production is unchanged in this PR and still builds every block locally. --builder-header-timeout is parsed and stored, but nothing reads it until the next PR, which adds the produceBlockV3 builder path and blinded_blocks. The flag docs already describe that behavior.

What it adds

Piece	Behavior
--builder <URL>	Checked while the flags are parsed, so a malformed URL, or one that is not http or https, is refused before the node starts. A one-off GET /eth/v1/builder/status runs in the background and only logs: nothing waits on it, and every later call to the builder is tried on its own. The user agent is the node's CLIENT_VERSION
--builder-header-timeout <ms>	Default 1000, at least 1, requires --builder
--builder-follow-redirects	Off by default, requires --builder. See Redirects
POST /eth/v1/validator/register_validator	JSON array or SSZ list. See below
lean_beacon_builder_registrations_total{outcome}	forwarded, failed, dropped, duplicate; plus call="status" and outcome="redirected" on lean_beacon_builder_request_seconds

register_validator

• Empty list: answered at once, without reading the head state. It is still a 500 with no --builder, as any other body is.
• Duplicates: one registration per validator is kept, its newest by timestamp, the later one on a tie. That is the one a builder keeps (builder-specs' process_registration). The rest are counted as duplicate, so one key repeated through a large body is forwarded once.
• Filter: keeps registrations whose validator the head state holds as active or pending (epoch < exit_epoch). The epoch is the later of the head's and the wall clock's.
  • Unknown and exited validators are dropped and counted, not refused, as beacon-APIs say.
  • The head read and the registry walk run on a blocking task, like the duty endpoints'. At most two walks run at once; further calls wait for a permit, so a burst of calls cannot fill the blocking pool.
• Fee recipients: each kept registration sets its validator's fee recipient, as prepare_beacon_proposer does, so a block built locally still pays the registered address. They are written before the builder is even looked up, so they hold when the builder fails or refuses, and with no --builder at all. That is Lighthouse's behavior. Prysm, Teku, Lodestar, Nimbus and Grandine set the local fee recipient only from prepare_beacon_proposer.
• Forward: one call, on its own task, so a validator client hanging up cannot cancel it. When nothing is left after filtering, no call is made (Prysm makes none either), so MEV-Boost does not fan [] out to every relay.
• Status codes:
  • a builder's 400 is passed through as a 400;
  • any other failure is a 500, and the error is logged;
  • with no --builder the answer is 500, so a validator client configured for a builder hears that it has none (as in Lighthouse, Teku and Lodestar);
  • a malformed body is a 400 either way.
• Trust: registrations are not BLS-verified here (none of Lighthouse, Prysm, Teku, Lodestar, Nimbus or Grandine verifies them); relays verify them. The caller can set fee recipients this way, just as with prepare_beacon_proposer. docs(rpc): say what the HTTP servers trust their callers with #778 documents what the HTTP servers trust their callers with.
• Future improvement: every call still walks the whole registry once. A pubkey-to-index map kept beside the head state, like Lighthouse's pubkey cache, would make it one lookup per registration.

Redirects

The builder client no longer follows redirects. #695's client followed up to ten, reqwest's default. Now a call goes only where --builder points, so registrations and signed blocks cannot be carried somewhere else. A redirect (301, 302, 303, 307 or 308) fails the call with BuilderError::Redirected. Its message names the target and the flag, for example:

builder redirected the request to https://relay.example/eth/v1/builder/validators (307), which was not followed; only --builder-follow-redirects follows a redirect, and then only a 307 or 308, up to ten, never from https to http

The target is resolved against the request URL and shown without any user or password; one that cannot be resolved to a URL with a host is shown as a placeholder, never as sent.

--builder-follow-redirects follows 307 and 308 only, the redirects that keep the method and body, up to ten hops and never from https to http. A 301, 302 or 303 would resend a POST as a body-less GET, so it fails the call even with the flag. Lighthouse's pre-gloas builder client and Prysm's relay client follow with their libraries' defaults; Lighthouse's gloas client follows none.

Tests

The tests run against the mock relay from #695, which can now also answer with a redirect:

• known, unknown, exited, pending and slashed-but-not-exited validators. The pending test pins the wall clock to slot 0; otherwise its validator would already be active at mainnet's epoch;
• duplicate registrations, and a timestamp tie;
• the empty-list skip, and an empty list with no builder;
• no builder, with fee recipients still recorded;
• a builder 400 passed through;
• an unreachable builder, on a released local port rather than port 1;
• fee recipients surviving builder failures;
• prepare_beacon_proposer and register_validator overwriting each other's fee recipient;
• redirects refused, followed when asked, and shown without credentials;
• SSZ bodies, malformed bodies, and 415;
• the CLI flags, including a bad --builder URL and --builder-follow-redirects.

cargo fmt --check and clippy (-D warnings, the rpc crate and the binary, all targets) are clean, and the rpc lib tests (387) and the binary's CLI tests pass.

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds builder API integration (MEV-Boost support) for validator registrations. Let me analyze the code carefully.

Overview

This PR adds:

1. CLI options for builder URL and timeout
2. Builder client status() endpoint for startup health check
3. /eth/v1/validator/register_validator endpoint implementation
4. Fee recipient management from registrations

---

Issues Found

1. Race Condition in Fee Recipients Update (Security/Correctness)

File: crates/net/rpc/src/beacon/registrations.rs, lines 127-132

{
    let mut recipients = fee_recipients.lock().expect("fee recipient lock poisoned");
    for (index, registration) in &kept {
        recipients.insert(*index, registration.message.fee_recipient);
    }
}
let kept: Vec<SignedValidatorRegistrationV1> = kept
    .into_iter()
    .map(|(_, registration)| registration)
    .collect();

The fee recipients are updated before the builder accepts the registrations. If the builder returns a 400 (bad signature), the fee recipients are still set (confirmed by test at line 442). This is intentional per the test, but the comment at line 15-16 says "a block built locally after all still pays the address the validator registered" — this is reasonable for liveness, but consider documenting this design choice more prominently since it affects validator rewards.

However, there's a subtle issue: if two concurrent registration requests for the same validator arrive with different fee recipients, the lock protects the HashMap, but the kept vector is built from one request while another request may interleave between the HashMap update and the builder call. This is probably acceptable given the HTTP handler concurrency model, but worth noting.

2. Unbounded HashMap Growth in Fee Recipients (Performance)

File: crates/net/rpc/src/beacon/registrations.rs, lines 127-130

Fee recipients are never pruned. A validator client re-registering thousands of validators every epoch will cause unbounded growth. The validator.rs module likely has similar behavior for prepare_beacon_proposer, but registrations can come from external validator clients with arbitrary pubkeys.

Suggestion: Add a TODO or bound the map size, or document that this matches existing prepare_beacon_proposer behavior.

3. Incorrect Error Classification for Unreachable Builder

File: crates/net/rpc/src/beacon/registrations.rs, lines 145-150

forwarded.map_err(|err| match err {
    BuilderError::BadRequest(message) => ApiError::BadRequestDetail(message),
    _ => ApiError::Internal("the builder did not take the registrations"),
})

An unreachable builder (connection refused) gets mapped to ApiError::Internal which becomes HTTP 500. However, the test at line 377 expects 500 for unreachable builder. Per beacon-APIs spec, this might be correct (the node can't fulfill the forwarding), but consider if 502/503 would be more appropriate. The current behavior is at least consistent with the "any other failure is a 500" documentation.

4. Blocking Operation in spawn_blocking Uses ? Incorrectly

File: crates/net/rpc/src/beacon/registrations.rs, lines 116-121

let kept = tokio::task::spawn_blocking(move || {
    let (_, state) = head(&store)?;
    let wall_epoch = compute_epoch_at_slot(super::node::wall_slot(&store));
    Ok::<_, ApiError>(active_or_pending(&state, registrations, wall_epoch))
})
.await
.map_err(|_| ApiError::Internal("checking the registrations failed"))??;

The ? inside spawn_blocking works because the closure returns Result. However, head(&store)? propagates ApiError correctly. This looks fine, but note that super::node::wall_slot(&store) is also called inside the blocking task — verify this doesn't need store access that conflicts with async runtime assumptions.

5. Missing Validation of Registration Signatures Before Forwarding

File: crates/net/rpc/src/beacon/registrations.rs, entire register function

The code does not verify BLS signatures on registrations before forwarding to the builder. It only checks:

• Validator exists in state
• Validator is active/pending

The builder is expected to reject bad signatures (test at line 349 confirms this). However, this means a validator client can submit registrations with invalid signatures, and the node will:

1. Set fee recipients (line 127-130)
2. Forward to builder
3. Return builder's 400 to the client

This is acceptable per spec ("must not reach the builder" is about unknown/exited validators, not invalid signatures), but the node could save a network round-trip by validating signatures locally. Not a bug, but a potential optimization.

6. Startup Builder Check is Fire-and-Forget Without Retry

File: bin/ethlambda/src/main.rs, lines 401-411

if let Some(builder) = builder.clone() {
    tokio::spawn(async move {
        let _ = builder
            .status()
            .await
            .inspect(|()| info!("Builder is up"))
            .inspect_err(|err| {
                warn!(%err, "Builder is not answering; proposals build locally until it does")
            });
    });
}

The status check runs once at startup with no retry. If the builder is temporarily down during node startup but comes up later, the log says "proposals build locally until it does" but there's no mechanism to actually retry. The builder client will still be used for get_header during proposals, which will fail individually.

Suggestion: The comment is misleading — proposals don't "build locally until it does" automatically; each proposal will still try the builder and fall back individually. Clarify the log message, or implement periodic health checks.

7. Clone of BuilderClient for Startup Check

File: bin/ethlambda/src/main.rs, line 401

if let Some(builder) = builder.clone() {

BuilderClient derives Clone and contains an Arc<Client> internally, so this is cheap. However, the builder variable is later moved into BeaconApiHandles at line 801. The clone is necessary because of the if let Some pattern — consider using if let Some(ref builder) = builder to avoid the clone, or verify the clone is truly cheap (it is, due to Arc).

Actually, looking more carefully: builder is Option<BuilderClient>, and at line 801 it's moved into the struct. The clone at line 401 is fine since BuilderClient is cloneable, but builder.as_ref() would avoid it entirely.

8. Test Helper store_with_pending Has Fragile Epoch Assumption

File: crates/net/rpc/src/beacon/registrations.rs, lines 463-470

fn store_with_pending(index: usize) -> Store {
    let mut state = with_signing_validators_at(ForkName::Fulu, VALIDATORS);
    let validator = state.validator_mut(index as ValidatorIndex).unwrap();
    validator.activation_epoch = 1_000;
    validator.exit_epoch = FAR_FUTURE_EPOCH;
    state.apply_pending_mutations();
    beacon_store_at(state).0
}

The test assumes epoch 0 is current (since wall_epoch will be 0 from default store). If with_signing_validators_at changes to initialize at a non-zero slot, this test could break. Consider being explicit about the state's slot.

9. Documentation Inconsistency

File: docs/metrics.md, line 449

call=get_header,register_validators,submit_blinded_block,status

But crates/net/rpc/src/beacon/builder_client/mod.rs line 72 uses submit_blinded_block_v2 or similar? Verify the metric label matches actual code. Looking at the code, observe_builder_request is called with "submit_blinded_block" in the existing code (not shown in diff), so this seems correct.

10. SSZ List Type Conversion

File: crates/net/rpc/src/beacon/registrations.rs, lines 169-172

BodyEncoding::Ssz => SignedValidatorRegistrations::from_ssz_bytes(body)
    .map(|list| list.to_vec())
    .map_err(|_| ApiError::BadRequest("invalid SSZ registrations")),

The SignedValidatorRegistrations type (SSZ list) is converted with to_vec(). Verify this doesn't allocate excessively for large lists. Also, the SSZ decoding doesn't enforce a maximum list length — a malicious client could send a very large SSZ list causing OOM. The DefaultBodyLimit at line 85 mitigates this to some degree.

---

Positive Observations

1. Good test coverage: Comprehensive tests for happy path, unknown validators, exited validators, pending validators, SSZ/JSON, missing builder, unreachable builder, bad signatures, malformed bodies, wrong content types, and wall-clock vs head-state epoch disagreement.

2. Proper use of spawn_blocking: The state read and validator iteration are correctly offloaded from the async runtime.

3. Metrics: Appropriate counter for registration outcomes with clear labels.

4. Error handling: SSZ/JSON decoding errors correctly return 400 before checking builder configuration.

5. Wall-clock epoch check: Correctly prevents lagging head from letting exited validators through (line 104-105, test at line 475).

---

Recommendations Summary

Priority	Item	Action
Low	6	Clarify startup log about "until it does" vs per-proposal fallback
Low	7	Use as_ref() instead of clone() in startup check
Low	2	Document or bound fee recipient map growth
Style	—	Add comment explaining why fee recipients are set even on builder failure

The code is generally well-structured and correct for consensus-layer considerations. No critical security vulnerabilities found.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 696: builder flag and register_validator

The PR is small, well scoped and well tested. I found no blocking issues, and a few minor points follow.

What looks right

• active_or_pending (registrations.rs:454) makes one registry pass through iter_validators(), as the CLAUDE.md registry guidance asks, rather than per-key tree descents. The epoch < exit_epoch test is correct, since pending validators still have FAR_FUTURE_EPOCH as their exit epoch.
• Taking the later of the head epoch and the wall-clock epoch is a sensible guard against a lagging head.
• The forward runs in its own tokio::spawn (:417), so a client hanging up cannot cancel the metric, the log or the call. The empty-list skip avoids fanning [] out to relays.
• Decoding before the builder check (:385) gives a 400 for a malformed body whether or not a builder is configured. The 500 for no builder is consistent with Lighthouse.
• The startup path fails on a malformed URL but only warns when the builder is unreachable, which is the right split. The metrics follow the lean_ convention.

Minor points

1. registrations.rs:405-410: fee recipients are written from unauthenticated, unverified registrations before the forward. The PR documents this and it matches prepare_beacon_proposer. Anyone who can reach --api-port can redirect a validator's fee recipient with a forged registration for a known pubkey, and the dropped/known filter doesn't prevent that. This is no worse than the existing endpoint, but both are exposed to the same risk and the API port shouldn't be public. A short security note in docs/rpc.md would help.
2. registrations.rs:407: if one request holds several registrations for the same pubkey, the last one wins locally, while all of them are forwarded to the builder. Nothing is wrong, but the order dependence is untested and undocumented.
3. registrations.rs:391: each call does an O(N) registry walk on spawn_blocking, with no concurrency bound. With about 1M mainnet validators and a misbehaving client, calls could pile up on the blocking pool. A 1-permit semaphore or a short cache keyed on the head root would bound it. This is low priority, since a validator client normally calls once per epoch.
4. main.rs:119: the warning says "proposals build locally until it does", but proposals build locally regardless in this PR. That is harmless now, though it will read oddly if someone looks at it before the next PR lands.
5. cli.rs: --builder is carried as a String and parsed in run_node. That works, and the error surfaces at startup. A value_parser for reqwest::Url would give a clearer CLI error, but it isn't needed.
6. inc_builder_registrations("dropped", 0) (:400) creates the label series with a zero increment. That is fine, but it will show the series as present even when nothing has been dropped.

The tests cover known, unknown, exited, pending, SSZ, 400 passthrough, unreachable and 415 cases, plus the wall-clock exit case. There is no test for duplicate pubkeys within one request (Point 2). I did not run the build or the test suite.

---

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

[github-actions]

🤖 Codex Code Review

Overall this looks solid. The builder wiring is reasonably contained, validates URLs at startup, keeps request-size bounds in the client, and the registration handler’s “drop unknown/exited, don’t fail whole request” behavior matches the Beacon API intent well.

• No correctness or security issue jumped out in fork choice, attestation processing, STF, XMSS, or SSZ code paths; this PR is narrowly scoped to builder RPC integration and stays out of consensus-critical logic.
• The main behavior change is in register_validator, and the active/pending filtering plus fee-recipient update are sensible.

Potential issue:

• crates/net/rpc/src/beacon/registrations.rs:89 updates FeeRecipients before the builder accepts the registrations, and the entries remain even if forwarding fails at crates/net/rpc/src/beacon/registrations.rs:103. If the builder is down or rejects the batch, the node may still locally build blocks paying those recipients despite the registration never reaching the builder. That may be intended for local-build fallback, but it changes semantics from “registered with builder” to “locally cached regardless of builder outcome.” If that is intentional, I’d call it out explicitly in code/docs; otherwise move the cache update after successful forwarding.

Nit:

• bin/ethlambda/src/main.rs:404 spawns a startup status probe and immediately discards the join handle. That’s fine for best-effort logging, but it means shutdown ordering is loose and the task may outlive early startup failures. Not a blocker, just something to keep in mind.

No other specific bugs found in the touched code.

---

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

[github-actions]

🤖 Kimi Code Review

⚠️ Review failed: Kimi API request failed with status 429: Your account org-00d0bbf7f87b44a7b4052af3f5fe367e is suspended due to insufficient balance, please recharge your account or check your plan and billing details

---

Automated review by Kimi (Moonshot AI)

[github-actions]

🤖 Kimi Code Review

⚠️ Review failed: Kimi API request failed with status 429: Your account org-00d0bbf7f87b44a7b4052af3f5fe367e is suspended due to insufficient balance, please recharge your account or check your plan and billing details

---

Automated review by Kimi (Moonshot AI)

[github-actions]

🤖 Claude Code Review

Review: PR 696 — register_validator and builder status probe

The PR is well-structured and well-tested. The decode-first ordering, the single-pass registry filter, and spawning the forward so a client hang-up can't cancel it are all good choices. My concerns are mostly about trusting unverified data.

1. Fee recipients are adopted from registrations the builder rejected as forged (medium)

crates/net/rpc/src/beacon/registrations.rs, in register:

• The fee-recipient map is written before the forward.
• The test fee_recipients_are_set_even_when_the_builder_refuses locks this behaviour in, using a registration with a deliberately wrong signature.
• So a request with any known pubkey, an arbitrary fee_recipient and a garbage signature changes the address that validator's locally built blocks pay. This holds even though the builder has just told us the signature is invalid.
• prepare_beacon_proposer is also unauthenticated, so this assumes a trusted API. Even so, register_validator carries a signature precisely so it can be checked. Ignoring a known-bad one is the wrong default.
• One bad entry also makes the builder 400 the whole batch, after every entry's recipient has been applied.

Suggested fixes, in order of preference:

• Verify each registration's BLS signature locally before keeping it. The domain is DOMAIN_APPLICATION_BUILDER with genesis_fork_version and a zero genesis_validators_root. Drop and count the invalid ones as "invalid". This also stops the node forwarding junk to relays.
• At minimum, skip the fee-recipient update when the builder answers BadRequest. Keep the update for timeout or unreachable, which is the case your other test rightly covers.

2. No builder means fee recipients are never set (minor, design)

builder.ok_or(...)? runs before the fee-recipient update. A VC configured with a builder flag against a node without --builder gets a 500 and its registered recipient is ignored. That is probably fine because VCs also call prepare_beacon_proposer. Worth a sentence in docs/rpc.md, since the module doc says registrations "also become" fee recipients unconditionally.

3. Startup probe wording (nit)

bin/ethlambda/src/main.rs:

• The message "proposals build locally until it does" implies the node gates on liveness. Nothing in the diff re-probes status or gates getHeader on it.
• Suggest "Builder is not answering at startup" or similar.
• The let _ = ….inspect(..).inspect_err(..) chain reads awkwardly. A plain match is clearer.

4. Metrics (nit)

inc_builder_registrations("dropped", 0) runs on every request with no drops. That is harmless. Guarding it with if dropped > 0 keeps the code intent obvious.

5. Body limit sanity check

DefaultBodyLimit::max(super::MAX_PUBLISH_BODY_BYTES):

• A JSON registration is about 450–500 bytes, so a VC with around 20k keys sends roughly 10 MB.
• Confirm MAX_PUBLISH_BODY_BYTES was sized with this endpoint in mind. If not, give it its own constant.

6. Looks correct

• Status filter: epoch < validator.exit_epoch correctly keeps the pending_*, active_ongoing, active_exiting and active_slashed statuses, and drops validators at or after their exit epoch. Using the max of the head epoch and the wall epoch is a nice touch.
• Spawned forward: the forward runs on its own task and its JoinError is mapped to 500. Good.
• CLI flags: the requires = "builder" and range(1..) constraints on the timeout are correctly tested. clap's requires does not fire on the default value, and the test confirms this.
• status probe: status follows the existing timing and outcome pattern.

---

Automated review by Claude · claude-opus-5-5 · custom prompt

[github-actions]

🤖 Codex Code Review

• [P1] Validate signatures before updating fee recipients — registrations.rs:89–93. A registration with a known validator’s public key, an arbitrary fee recipient, and an invalid signature overwrites the local recipient before the relay rejects it. Subsequent locally built blocks use that address, potentially redirecting execution rewards despite the request returning 400. Verify registrations locally before mutating FeeRecipients, and change fee_recipients_are_set_even_when_the_builder_refuses to assert that invalid signatures leave the map unchanged.

Tests could not run because rustup attempted to write to a read-only directory.

---

Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt

[github-actions]

🤖 Gemini Code Review

This PR looks solid and well-tested. The CLI flag validation, startup status probe, register_validator filtering logic (including wall-clock epoch handling for lagging heads), and documentation updates are all clean and match the Beacon and Builder API specs.

One minor suggestion:

1. Short-circuit empty registration batches before loading head state (crates/net/rpc/src/beacon/registrations.rs:70-87)
   When a validator client sends an empty registration list ([]), register currently still spawns a blocking task, clones the head BeaconState, and scans the entire validator registry inside active_or_pending even though wanted is empty. Returning Ok(()) right after checking builder when registrations.is_empty() would avoid the unnecessary state clone and registry walk:

   let registrations = decode(headers, body)?;
   let builder = builder.ok_or(ApiError::Internal("no builder is configured (--builder)"))?;
   if registrations.is_empty() {
       return Ok(());
   }

---

Automated review by Gemini · gemini-4-argon · custom prompt

[github-actions]

🤖 Claude Code Review

Review: PR 696, register_validator and builder status

The PR is well structured and well tested. The doc comments explain why, not just what. Notable strengths:

• Decoding happens before the builder check, so a malformed body is a 400 whether or not a builder is configured.
• The registry walk runs in spawn_blocking.
• The forward runs on its own task, so a client hanging up does not abort it.
• The exit check uses the later of the head's epoch and the wall clock's.

A few points below.

Correctness and security

1. Forged registrations still set fee recipients. In registrations.rs, register writes the fee recipients before the builder has seen the batch. The test fee_recipients_are_set_even_when_the_builder_refuses locks this in, even for a registration the builder rejected as invalid signature.

   • This matches Lighthouse's behaviour.
   • prepare_beacon_proposer is equally unauthenticated, and the beacon API is a trusted interface, so this is not a new attack surface.
   • Still, it is worth stating explicitly in the module doc that signatures are not verified locally and that the fee recipient is trusted from the request.
   • Alternatively, insert the recipients only when the forward returns Ok or a non-400 error. Then a builder-confirmed bad signature does not redirect local block rewards.

2. Exit check is correct. active_or_pending uses epoch < validator.exit_epoch, which covers pending_*, active_ongoing, active_exiting and active_slashed. It also correctly excludes exited_* and withdrawal_*.

3. Duplicate pubkeys in one request are all forwarded, and the last one wins in the fee-recipient map. This is harmless, but a dedup keyed on pubkey would keep the builder call minimal. Optional.

Performance

4. Full registry scan on every call. active_or_pending does state.iter_validators() over the entire registry on every call. VCs send this every epoch, and on mainnet the registry has about 2M entries.

   • The single pass with a HashSet filter is fine.
   • If the store already keeps a pubkey→index cache (as the duty endpoints likely do), use it here and skip the scan.

5. Body limit. DefaultBodyLimit::max(super::MAX_PUBLISH_BODY_BYTES): a JSON registration is about 450–500 bytes, so 10k keys is about 5 MB. Confirm the limit covers large VC deployments. Otherwise give this route its own explicit limit.

Startup probe (main.rs)

6. Log message implies tracked state. The warning "proposals build locally until it does" suggests the node tracks builder liveness, but nothing consumes the probe result. Proposals will still try getHeader and then fall back on failure. Suggested wording: "Builder is not answering; proposals will fall back to local building if it stays down".

7. Awkward let _ = ... .inspect(...) chain. A plain match (or if let Err) reads more naturally than discarding a Result built only for side effects:

   match builder.status().await {
       Ok(()) => info!("Builder is up"),
       Err(err) => warn!(%err, "Builder is not answering; ..."),
   }

CLI

8. requires with a default works as intended. requires = "builder" combined with default_value_t = 1000 behaves correctly, because clap only enforces requires on explicitly supplied args. The tests cover both directions.

Minor

9. Unnecessary metric series. inc_builder_registrations("dropped", 0) creates a zero-valued series on every call. This is harmless, but you could guard it with if dropped > 0 if you prefer not to touch the metric needlessly.

10. Possible test flakiness. an_unreachable_builder_is_a_server_error relies on 127.0.0.1:1 refusing connections. That is fine on CI, but binding and dropping a listener to get a guaranteed-closed port is slightly more robust.

Overall the PR looks good. Item 1 is the only one I'd want a deliberate decision on, even if that decision is just documenting it.

---

Automated review by Claude · claude-opus-5-5 · custom prompt

[github-actions]

🤖 Codex Code Review

• [P1] Invalid registrations overwrite the local fee recipient — registrations.rs:89–93. The cache is updated before signature verification or builder acceptance. A registration with a forged signature and arbitrary fee recipient therefore changes where locally built blocks pay rewards, even though the endpoint returns 400. The test fee_recipients_are_set_even_when_the_builder_refuses confirms this behavior. Verify signatures before updating local recipients; valid registrations can still update the cache when the builder is unavailable.

Tests could not run because Rust toolchain initialization requires writing outside the permitted filesystem.

---

Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt

[github-actions]

🤖 Gemini Code Review

This PR is clean, well-structured, and thoroughly tested. The CLI flag validation, startup builder status check, registration filtering against max(head_epoch, wall_epoch), fee recipient fallback handling, and metric instrumentation all look solid.

One minor suggestion:

1. Short-circuit empty registration lists before loading the head state (crates/net/rpc/src/beacon/registrations.rs:74-87)
   If a client sends an empty array [], register currently spawns a blocking task, loads the head BeaconState, and scans the validator registry before returning at if kept.is_empty(). Checking if registrations.is_empty() { return Ok(()); } right after verifying builder.ok_or(...)? would avoid unnecessary state cloning and registry iteration on empty requests.

---

Automated review by Gemini · gemini-4-argon · custom prompt

[github-actions]

🤖 DeepSeek Code Review

⚠️ Review failed: DeepSeek API request failed: curl exit code 56

---

Automated review by DeepSeek

[github-actions]

🤖 Grok Code Review

The registration path is well structured. It decodes before the builder check, scans the registry once on spawn_blocking, does not hold the fee-recipient lock across the await, and the exit check uses the later of the head epoch and the wall clock. The fee-recipient write is not safe as written.

1. crates/net/rpc/src/beacon/registrations.rs around lines 89–103 never verifies SignedValidatorRegistrationV1 before side effects. active_or_pending only checks pubkey membership and exit_epoch. The fee-recipient map is then updated, and the batch is forwarded, with no builder-domain signature check. fee_recipients_are_set_even_when_the_builder_refuses locks that in: a forged signature still overwrites validator 0's fee recipient, and the unreachable-builder path applies the same write before the 500. This endpoint is what local payload attributes pay. Any caller who can hit it can redirect tips for a known pubkey until the next honest prepare_beacon_proposer or register_validator. Verify each signature against the application-builder domain, using the genesis fork version and genesis validators root, before the map write and before register_validators. Do not mutate or forward failures. Returning 400 with no side effects matches the beacon API. Updating fee recipients when the builder is down is fine only after that local check. Those two tests need valid signatures.

2. Same file, lines 144–162 and 89–98: duplicate pubkeys are all kept, forwarded in request order, and the fee-recipient map keeps the last insert, not the greatest timestamp. Builder spec keeps the latest timestamp, so a locally built block can pay a different address than the bid. Dedupe by pubkey, keep the highest timestamp, and only then update recipients and forward.

3. Startup status is correctly non-fatal, URL parsing fails closed, the body limit and clap requires and range checks are right, and the cancellation-safe forward is sound. No fork-choice or state-transition issues in this diff.

---

Automated review by Grok (xAI) · grok-4.7 · custom prompt

[github-actions]

🤖 Grok Code Review

The registration path updates local fee recipients from unverified input. That is a reward-theft bug on any beacon API that is not strictly VC-only. The rest of the wiring (CLI, status probe, exit filtering, metrics, tests) is in good shape.

1. Unverified registrations rewrite fee recipients. In crates/net/rpc/src/beacon/registrations.rs around lines 89-94, every kept registration is inserted into fee_recipients before register_validators, and there is no BLS check. fee_recipients_are_set_even_when_the_builder_refuses locks that in: a forged signature still sets the address, then the handler returns 400. This map is what local payload building pays. register_validator is on the same router as the public beacon API (lib.rs just adds Extension(handles.builder)), and the signature is the only authorization this message has. Anyone who can POST here can point proposal tips at their address; a builder outage is the worst case, because nothing else rejects the forgery. Verify each signature in the existing spawn_blocking walk (builder domain, genesis fork version), drop failures, and only then update the map. Also reject timestamps far in the future.

2. Fee-recipient updates are not ordered, and they stick after a failed forward. Lines 91-92 last-write-wins in request order, with no timestamp. A replay of an older valid registration, or a duplicate later in the same body, overwrites a newer fee recipient. The same block commits the whole batch before the builder answers (lines 89-118), so one bad entry still leaves every other address updated while the VC sees 400/500 and retries. Keep a timestamp next to the address and apply a registration only after it verifies and its timestamp is greater than or equal to the stored one. Do not treat a builder rejection as a successful local update.

3. A lagging or syncing head silently succeeds. Unknown keys are dropped and an empty keep-set returns 200 (lines 85-86) without calling the builder. While the head is behind, a validator that is not in that state yet is indistinguishable from an unknown key, so the VC will not retry until its next epoch. If head is not synced, return 503 instead of counting everyone as dropped.

4. Minor: --builder-header-timeout can panic at startup. cli.rs line 324 allows any u64 from 1 up, and main.rs line 462 passes it to Duration::from_millis, which panics above u64::MAX / 1_000_000. Cap the clap range at a few seconds (or use Duration::try_from_millis).

Exit filtering itself looks right: epoch < exit_epoch at max(head, wall) matches “active or pending,” and the pending, exited, and wall-clock tests cover the boundary. The detached forward so a client hang-up still records metrics is also correct.

---

Automated review by Grok (xAI) · grok-4.7 · custom prompt

[github-actions]

🤖 DeepSeek Code Review

Overall, this PR is well-structured and test-heavy. The main blocker is a security-sensitive ordering issue in the new register_validator endpoint.

Critical

• Unverified registrations overwrite local fee recipients before the builder validates them (crates/net/rpc/src/beacon/registrations.rs, around lines 88-93).
  register filters active/pending validators, then immediately calls:

  recipients.insert(*index, registration.message.fee_recipient);

  and only afterwards forwards to the builder. If the builder rejects the registration due to a bad signature, is unreachable, or returns 500, the local fee-recipient map still retains the value. The tests even assert this behavior:

  • fee_recipients_are_set_even_when_the_builder_refuses
  • fee_recipients_are_set_even_when_the_builder_is_unreachable

  This means anyone who can reach the beacon API can submit an arbitrary public key plus an invalid signature for an active validator, get the builder to reject it, and still overwrite that validator’s fee recipient for locally built blocks. Since register_validator payloads are signed precisely so the builder can authenticate them, the local node should not trust them for fee recipients unless it also verifies the signature locally.
  Suggested fix: verify registration signatures locally against the head-state pubkey before inserting fee recipients, or only update fee recipients after the builder accepts the registration, or do not merge unverified register_validator calls into the local FeeRecipients map at all.

Minor / clarity

• tokio::spawn(...).await does not match the comment in registrations.rs around lines 100-115. The comment says the forward must complete regardless of the client hanging up, but the handler immediately .awaits the JoinHandle, so a normal request still waits for the builder. The spawn only protects the task if the handler future is dropped before the await. That may be intentional, but the comment should say so more clearly.

• Poisoned FeeRecipients lock panics around line 88.
  .lock().expect("fee recipient lock poisoned") converts one panic into repeated panics for every subsequent registration request. Consider lock().unwrap_or_else(|poison| poison.into_inner()) unless poisoning is treated as fatal elsewhere.

• Test reliability: an_unreachable_builder_is_a_server_error relies on http://127.0.0.1:1 refusing connections immediately. This is usually fine, but can be flaky on systems where port 1 is filtered or connection timeouts differ. A local listener that immediately drops connections would be more deterministic.

• No local signature verification is intentional for forwarding, but the endpoint should be explicit that the builder is the authority for builder acceptance. That distinction is lost when the same payload mutates local block-building state.

---

Automated review by DeepSeek · deepseek-v4-pro · custom prompt

[pablodeymo]

Security read of this PR (and the client it inherits from #695). No critical or high issues; the trust model is the same class as prepare_beacon_proposer and matches Lighthouse. Two things I'd fix before merging, plus a few smaller ones.

1. Dedup registrations by pubkey before forwarding (registrations.rs, active_or_pending)

The route takes MAX_PUBLISH_BODY_BYTES (64 MiB) and keeps every registration whose pubkey is active or pending, with no dedup. One real pubkey repeated ~370k times (180 B each in SSZ) with garbage signatures passes the filter, is re-encoded as JSON (~150 MB) and forwarded on the detached task for up to REGISTER_TIMEOUT. Concurrent requests multiply the memory, and relays refuse the whole batch on the first bad signature, so any legitimate registrations in the same batch go down with it. Keeping the latest timestamp per pubkey (what relays do anyway) closes this; a lower body limit for this route would help too, since a mainnet-sized key set is far below 64 MiB.

2. Short-circuit an empty body

register spawns the registry walk even when registrations is empty: wanted is empty, the result is known, and the whole iter_validators() pass still runs on the blocking pool. A flood of [] bodies is a cheap way to pin the pool the duty endpoints share. An if registrations.is_empty() { return Ok(()) } before spawn_blocking is enough.

Smaller

• Fee recipients are set from unsigned registrations before, and regardless of, the builder's verdict. That is intended and the same trust prepare_beacon_proposer already extends to any API caller, but it is worth a sentence in docs/rpc.md next to the endpoint so operators know the API must stay on loopback or behind a proxy.
• The reqwest client in BuilderClient::new follows redirects and honours HTTP_PROXY/HTTPS_PROXY. A 30x from the builder silently retargets registration bodies. .redirect(Policy::none()) would make the destination exactly what --builder says. (feat(rpc): a builder API client (getHeader, registerValidator, submitBlindedBlockV2) #695 code, but this PR is the first to send bodies through it from an API handler.)
• The builder's 400 message (up to 1 KiB) is reflected verbatim into the validator client's 400 and into the warn!. Truncated, not sanitised; fine if we accept that a hostile relay can write into our logs.

Tests I'd add: duplicate pubkeys in one request; an empty [] body; a slashed-but-not-exited validator being kept; prepare_beacon_proposer then register_validator on the same index (last writer wins).

Checked and clean: the builder URL never reaches errors or logs (without_url everywhere, InvalidUrl carries only the parser's text); the dropped count cannot underflow; both spawned tasks map join errors to a 500; the exit check at the later of head and wall-clock epoch keeps pending/active/exiting/slashed and drops exited; builder responses are capped at 8 MiB with per-call timeouts; the Option<BuilderClient> extension is layered on the one production router that mounts the route.

[MegaRedHand]

Thanks for the reviews. 6fb5e57 addresses most of them, and the updated description has the details:

• fee recipients are now recorded with no builder too;
• duplicate registrations are reduced to the one with the newest timestamp;
• an empty list is answered without walking the registry;
• redirects are off by default, behind --builder-follow-redirects;
• --builder is checked while the flags are parsed;
• the startup log is reworded;
• @pablodeymo's tests are added.

The trust note is in #778. These are the points we did not take, and why.

Verifying registration signatures before setting fee recipients (Codex, Grok, DeepSeek, Claude, Kimi)

• We kept Lighthouse's behavior. The fee recipient is written before the builder is asked, whatever it answers, so a block built locally still pays the registered address.
• None of Lighthouse, Prysm, Teku, Lodestar, Nimbus or Grandine verifies these signatures in the beacon node. The relays do (builder-specs' process_registration).
• Verifying here would not close the hole the reviews describe either. prepare_beacon_proposer sets the same fee recipient with no signature at all, so the defense is keeping the API off the open network, which docs(rpc): say what the HTTP servers trust their callers with #778 documents.
• The other five clients don't set the local fee recipient from registrations at all. Lighthouse does, so a locally built block falls back to the registered address, and we matched it.

A body limit of its own (pablodeymo, Claude, Kimi)

• Left on the shared 64 MiB for now.
• The amplification pablodeymo described, one key repeated ~370k times, is gone with the dedup: each key is forwarded once.
• For comparison: Lighthouse, Prysm and Grandine set no limit on this route, and Lodestar's 20 MiB and Nimbus's 16 MiB are global limits.

The registry walk on every call (Claude)

• Noted as a future improvement in the code and in docs/rpc.md.
• A pubkey-to-index map beside the head state would make it one lookup per registration. ethlambda has no such map yet, so building one is beyond this PR.
• The empty-body short-circuit removes the cheapest way to trigger the walk.

A 503 while the head lags (Grok)
beacon-APIs lists only 200, 400, 415 and 500 for this endpoint. Lighthouse, Prysm, Lodestar and Nimbus have no sync guard here.

Timestamp checks beyond the dedup (Grok)

• Grok also asked for two more checks: rejecting far-future timestamps, and ordering fee-recipient writes by timestamp across requests.
• Relays already enforce both the lookahead and monotonic timestamps. No beacon node does either.
• prepare_beacon_proposer carries no timestamp to order against, so the last write wins, as in Lighthouse.

The startup probe (Claude, Kimi)

• The wording is fixed. The chain of inspect/inspect_err stays rather than becoming a match, because CLAUDE.md prefers it for side-effect-only handling.
• No periodic status check is added. Nothing gates on the builder's status, and each call is tried on its own, as in Lighthouse, Nimbus and Grandine. Teku and Lodestar do gate on it.

Doesn't apply

• --builder-header-timeout panicking in Duration::from_millis (Grok): it takes any u64 without overflow.
• as_ref() instead of clone() for the probe (Kimi): tokio::spawn needs an owned, 'static value.
• An unbounded fee-recipient map (Kimi): its keys are indices of validators in the head state, so the registry bounds it.
• 502 or 503 instead of 500 (Kimi): neither is in the endpoint's spec.
• spawn(...).await contradicting its comment (DeepSeek): the comment describes the case where the handler is dropped, which is what the spawn is for.
• expect on a poisoned lock (DeepSeek): the same as in prepare_beacon_proposer and the rest of the crate.
• The detached startup probe (Codex), and inc_by(0) creating the dropped series (Claude): both harmless.
• The builder's 400 message being passed back (pablodeymo): accepted in the review, and Lighthouse does the same.

[MegaRedHand]

/review

[github-actions]

🤖 Gemini Code Review

This PR is clean, well-structured, and thoroughly tested.

A few highlights of what looks good:

• Redirect handling (crates/net/rpc/src/beacon/builder_client/): Disabling redirects by default (Policy::none()) unless --builder-follow-redirects is explicitly enabled prevents leaking validator registrations or signed blinded blocks to unintended targets, while shown_location properly strips credentials before surfacing the Location header in errors/logs.
• Validator registration pipeline (crates/net/rpc/src/beacon/registrations.rs): Deduplicating by pubkey (keeping the newest timestamp, breaking ties toward the later entry), filtering against max(head_epoch, wall_epoch) < validator.exit_epoch on the blocking pool, recording fee recipients prior to builder forwarding, and isolating the outbound builder request in a spawned task so client disconnects don't abort the forward or its metrics/logging are all well-handled.
• CLI & Docs (bin/ethlambda/src/cli.rs, docs/): Early URL validation in clap (parse_builder_url) and flag constraints (requires = "builder", non-zero timeout range) match the documented behavior and metrics.

No issues found — LGTM.

---

Automated review by Gemini · gemini-4-argon · custom prompt

[github-actions]

🤖 Claude Code Review

Review of PR 696: builder registrations, redirect policy and status probe

Overall this is well structured and tested. The redirect-off default, the credential-stripped redirect target, the dedup by timestamp and the max(head_epoch, wall_epoch) exit check are all good choices. A few points below, roughly in priority order.

Correctness and security

1. Following redirects can silently change POST to GET (builder_client/mod.rs, with_redirects).

   • Policy::default() follows 301, 302 and 303 as well as 307 and 308. For 301, 302 and 303, reqwest rewrites a POST into a GET and drops the body.
   • With --builder-follow-redirects, a builder answering 302 to submitBlindedBlockV2 would get a body-less GET. The resulting error is confusing, and it costs a proposal.
   • It also follows an https→http downgrade.
   • Suggested fix: a custom policy that follows only method-preserving redirects and refuses scheme downgrades:

     Policy::custom(|attempt| {
         let keeps_method = matches!(attempt.status().as_u16(), 307 | 308);
         let downgrade = attempt.previous().last().is_some_and(|p| p.scheme() == "https")
             && attempt.url().scheme() == "http";
         if !keeps_method || downgrade || attempt.previous().len() >= 10 {
             attempt.stop()
         } else {
             attempt.follow()
         }
     })

     A stopped redirect then reaches from_status and becomes Redirected, which is the right error. Also update the docs/cli.md wording ("up to ten").

2. shown_location misses scheme-relative and relative targets (builder_client/error.rs, shown_location).

   • Url::parse("//user:pass@host/x") fails as a relative URL, so the raw value, credentials included, ends up in the error and the logs.
   • Suggested fix: resolve against the response URL and strip credentials from the result. This also gives operators an absolute target for relative Locations:

     let location = match response.url().join(&raw) {
         Ok(mut url) => { let _ = url.set_username(""); let _ = url.set_password(None); url.to_string() }
         Err(_) => raw,
     };

3. A forged registration overwrites the fee recipient, even when the builder rejects it (registrations.rs, the fee-recipient write and the test fee_recipients_are_set_even_when_the_builder_refuses).

   • This matches Lighthouse, and prepare_beacon_proposer has the same trust model, so I'm not asking for a change.
   • The consequence should be stated explicitly in docs/rpc.md, though: anyone who can reach the validator API can redirect a validator's locally built block rewards with an unsigned registration. The API must not be exposed.
   • Related: nothing orders registrations across requests. A replayed older registration (lower timestamp) still overwrites a newer recipient. Consider keeping the last-applied timestamp per index and skipping older ones, at least for the registration path.

4. The unbounded registry walk is a DoS vector (registrations.rs, active_or_pending / spawn_blocking).

   • This is already documented as a future improvement. On mainnet, though, it is about 2M validators per request, and it shares the blocking pool with the duty endpoints.
   • A cheap interim fix: a tokio::sync::Semaphore with 1–2 permits around the spawn_blocking. Concurrent register calls then queue rather than saturate the pool.

Minor / nits

• Startup probe (main.rs): let _ = builder.status().await.inspect(..).inspect_err(..) reads awkwardly. A plain match is clearer:

  match builder.status().await {
      Ok(()) => info!("Builder is up"),
      Err(err) => warn!(%err, "..."),
  }

• from_status treats every 3xx as Redirected, including 304. That is harmless, but "redirected to nowhere (no Location header) (304)" is slightly misleading. Consider limiting it to statuses that carry a Location, or accept as is.
• inc_builder_registrations("duplicate", 0) and ("dropped", 0) are called on every request. That is fine (it pre-creates the series), just noting it is intentional.
• BuilderClient derives Debug with base_url. If --builder carries basic-auth credentials and the client is ever {:?}-logged, they leak. Consider a manual Debug that redacts the URL, mirroring from_reqwest's care.
• CLI tests: they are good. requires = "builder" combined with a default_value_t works as intended, because clap only enforces requires on explicit values, and a_header_timeout_needs_a_builder covers it.

Tests

The coverage is solid: dedup tie-break, wall-clock exit, pending and slashed validators, redirect credentials, and the fee-recipient interplay with prepare_beacon_proposer. Using unreachable_url() instead of port 1 is a nice robustness fix. If Item 1 is adopted, add a test that a 302 on POST is not followed.

---

Automated review by Claude · claude-opus-5-5 · custom prompt

[github-actions]

🤖 Codex Code Review

1. [P2] Bound concurrent registry scans — registrations.rs:95. Every nonempty request scans the entire validator registry, even for one unknown pubkey or without a builder configured. Repeated small requests can saturate the shared blocking pool and delay validator duties. Add bounded admission before spawning work, or use a cached pubkey-to-index map.

2. [P2] Redact credentials in scheme-relative redirects — error.rs:117. A valid Location: //operator:secret@relay.example/path fails Url::parse and is logged verbatim, exposing credentials. Resolve the location against response.url() before stripping user information, and add a regression test.

Tests could not run because rustup attempted to write to a read-only directory.

---

Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt

[github-actions]

🤖 Kimi Code Review

1. High — forged registrations can overwrite validator fee recipients (crates/net/rpc/src/beacon/registrations.rs:106-115). Registration signatures are never verified, yet fee recipients are updated before the builder is consulted—and even when no builder exists. Consequently, any caller able to reach the validator API can set an arbitrary fee recipient for any active/pending validator and divert rewards from locally built payloads. The tests using forged registrations currently confirm this behavior. Verify each registration against the validator’s pubkey and builder domain before mutating FeeRecipients, or stop using this endpoint to update local fee recipients. At minimum, a rejected/forged registration must not change the cache.

2. Medium — unbounded, attacker-triggerable full-registry scans (registrations.rs:95-104, active_or_pending). Every non-empty request queues a spawn_blocking task that walks the entire validator registry; there is no concurrency bound, and this work occurs even without a configured builder. Repeated requests can exhaust the blocking pool and CPU, especially on large mainnet states. The comment acknowledges the scan cost but not the unbounded queue. Add a semaphore/rate limit and preferably cache a pubkey-to-index map keyed by head state. Bounding registrations per request and concurrent builder-forwarding tasks would also help.

3. Low — redirect credential redaction misses scheme-relative URLs (builder_client/error.rs, shown_location). A valid Location: //user:password@example.com/path fails Url::parse, so the raw value—including credentials—is returned and logged. Resolve the header with response.url().join(&location) before stripping username/password; this handles absolute, scheme-relative, and relative redirects consistently. Add a scheme-relative credential test.

The CLI validation, secure default of refusing redirects, and otherwise extensive test coverage look good.

---

Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt

[github-actions]

🤖 Grok Code Review

The registration path and redirect defaults are in good shape: malformed bodies stay 400, redirects are off unless asked, credential stripping is tested, and the forward is detached so a client hang-up does not drop it. A few correctness and safety gaps are worth fixing before this is relied on for local block production.

1. Fee recipient is last-write-wins, not newest-wins, across requests. newest_per_validator only dedupes inside one body (registrations.rs around the timestamp >= check). The map update then stores only the address:

        for (index, registration) in &kept {
            recipients.insert(*index, registration.message.fee_recipient);
        }

That runs before the builder answers, and a later request with an older timestamp overwrites a newer one. Concurrent requests race the same way (lock order, not timestamp). The builder will keep the newer registration (or return 400), while a locally built block pays the stale address. Keep (timestamp, fee_recipient) and insert only when timestamp is greater than or equal to the stored one, under the same lock.

2. A lagging head can still forward exited validators. The module comment says judging exit at max(head_epoch, wall_epoch) stops a lagging head letting exits through. That only works if exit_epoch is already in the head state (active_or_pending, epoch < validator.exit_epoch). Exits processed on the canonical chain after this head are still FAR_FUTURE_EPOCH, so wall_epoch does not filter them and they are forwarded. Beacon-API says those must not reach the builder. While head is more than a slot or two behind the clock, return 503 and do not forward, or otherwise refuse until the head is caught up.

3. --builder-header-timeout can panic at startup. The clap range is 1.. (cli.rs), and startup does Duration::from_millis(mainnet.builder_header_timeout) (main.rs). from_millis panics when millis > u64::MAX / 1_000_000. Cap the parser at that bound (a few seconds to a minute is enough for a bid wait).

4. --builder-follow-redirects follows any host. Policy::default() (BuilderClient::with_redirects) will send registrations and signed blinded blocks to wherever Location points, including another host or an internal URL. Default-off is the right call. If the flag stays, restrict follows to the same origin as --builder so a compromised relay cannot bounce the signed submission elsewhere. shown_location also fail-opens: if Url::parse fails, or set_username / set_password returns Err, the raw Location (userinfo included) is logged. On failure, log a fixed redacted placeholder.

The unbounded full-registry walk on every register_validator is real availability pressure on the shared blocking pool; the comment already tracks that. A concurrency cap would be enough until a pubkey index exists. Signature checks staying at the builder match the stated Lighthouse behavior; this endpoint must stay on the trusted validator API, since it can retarget fee recipients without a signature.

---

Automated review by Grok (xAI) · grok-4.7 · custom prompt

[github-actions]

🤖 DeepSeek Code Review

Overall the builder URL handling, redirect policy, status probe, and registration endpoint are well documented and well tested. No consensus state-transition or SSZ changes are touched, so I don’t see fork-choice/finality issues in this PR.

A few concerns and nits:

1. crates/net/rpc/src/beacon/registrations.rs around line 154: fee recipients are written before the builder verifies signatures, and they persist even when the builder returns 400, is unreachable, or no builder is configured. This matches Lighthouse behavior and is documented, but it means anyone who can reach this beacon API endpoint can set fee recipients for active/pending validators without a valid signature for that validator. Since prepare_beacon_proposer already has the same trust model this isn’t a new security boundary, but operators should be aware.

2. crates/net/rpc/src/beacon/registrations.rs around lines 151 and 232: every non-empty registration request walks the entire validator registry on spawn_blocking, with no concurrency limit. A burst of register_validator requests can tie up the blocking pool used by duty endpoints. The docs acknowledge this as a future improvement; consider adding a semaphore or a pubkey-to-index cache before exposing this endpoint broadly.

3. crates/net/rpc/src/beacon/builder_client/mod.rs around line 106: Policy::default() follows up to ten redirects and can forward signed block/registration bodies to a different host when --builder-follow-redirects is enabled. The default-off behavior is safe, but when enabled, consider logging a warning and possibly restricting redirects to same-origin or HTTPS.

4. crates/net/rpc/src/beacon/builder_client/error.rs around line 73: redirect responses are returned without draining the response body. Dropping an unread reqwest::Response can prevent connection reuse; draining the body before returning BuilderError::Redirected would be cleaner.

5. crates/net/rpc/src/beacon/registrations.rs around line 157: the tokio::spawn(async move { ... }).await pattern is unusual. It does provide cancellation safety because dropping the JoinHandle detaches the task, but the handler still waits synchronously for the builder call to finish. If this is meant to be fire-and-forget, drop the handle; if the client needs the result, a direct await or explicit timeout/select would be clearer.

6. Minor: crates/net/rpc/src/beacon/registrations.rs around the builder.ok_or(NO_BUILDER)? check before if kept.is_empty() means all-dropped requests with no builder return 500, while all-dropped requests with a builder return 200. That’s documented for the empty-list case, but a comment next to that asymmetry would help future readers.

---

Automated review by DeepSeek · deepseek-v4-pro · custom prompt

[MegaRedHand]

Thanks for the second round. The changes are in f33971d. I checked each point against Lighthouse, Prysm, Teku, Lodestar and Nimbus.

Changed

• Credentials in a redirect's target (Claude, Codex, Kimi, Grok).
  • The fix: Location is now resolved against the request URL, then stripped of any user and password. A scheme-relative //user:pass@host/x no longer reaches the logs, and relative targets show as absolute.
  • A target that does not resolve to a URL with a host, or cannot be stripped, shows as a fixed placeholder, never as sent.
  • Tests cover a scheme-relative target with credentials, a relative path, an unparsable value, and credentials from --builder not being inherited.
• What --builder-follow-redirects follows (Claude, Grok, DeepSeek).
  • The rule: only 307 and 308, which keep the method and body, and never from https to http, for at most ten hops. A 301, 302 or 303 used to turn a POST into a body-less GET; it now stops and comes back as Redirected.
  • How others do it:
    • Lighthouse's pre-gloas builder client and Prysm's relay client follow with their library defaults (301–303 included, no downgrade check).
    • Lighthouse's gloas client follows none, citing the spec's "MUST NOT follow redirects".
  • Ours is off by default, and stricter than either default when turned on.
  • Tests:
    • 301, 302 and 303 on a POST are refused;
    • 307 and 308 are followed with the body intact;
    • a loop stops after ten hops;
    • the downgrade rule is unit-tested, since the mock relay is plain HTTP.
• Only real redirects count as redirects. 301, 302, 303, 307 and 308 are Redirected. A 300 or 304 is the ordinary status error.
• Concurrent registry walks (Claude, Codex, Kimi, Grok, DeepSeek).
  • Each walk now holds one of two permits, so concurrent register_validator calls queue instead of filling the blocking pool. The permit moves into the blocking task, so a caller that hangs up does not release it early.
  • Lighthouse avoids the walk with its pubkey cache, which stays the follow-up noted in the code.
• BuilderClient's Debug (Claude): written out by hand, and it prints --builder without its credentials.
• The 500/200 asymmetry (DeepSeek): commented. A request whose every entry was dropped answers 500 without a builder and 200 with one. The 500 is how a client set up for a builder learns that there is none.

Checked, not changed

• Fee recipients from unsigned registrations (Claude, Kimi, Grok, DeepSeek): this is Lighthouse's behaviour. It calls update_proposer_preparation before it forwards to the builder (http_api/src/validator/mod.rs:783 vs :811), and prepare_beacon_proposer has the same trust model. docs(rpc): say what the HTTP servers trust their callers with #778 writes that model down in docs/rpc.md.

• No timestamp order across requests (Claude, Grok): Lighthouse is last-write-wins too. prepare_beacon_proposer carries no timestamp, so there is nothing to order it against.

• Exits a lagging head has not seen yet (Grok): exits the head state knows of are judged at max(head epoch, wall epoch), so a lagging head still drops them. What a lagging head cannot know is an exit processed after it. Four of the five clients forward while syncing:

  • Lighthouse wires register_validator without its not_while_syncing filter, which prepare_beacon_proposer has;
  • Prysm, Lodestar and Nimbus have no check.

  Teku refuses with a 500 while syncing. I kept forwarding, with the majority.

• A panic from a large --builder-header-timeout (Grok): there is none. Duration::from_millis takes any u64, and tokio's sleep saturates a deadline that overflows (checked_add, then far_future).

• Draining a redirect's body, the spawned forward, the startup probe's style (DeepSeek, Claude):

  • the redirect's body is not drained, and that only costs connection reuse on an error path;
  • the forward is spawned on purpose, so a caller that hangs up does not cancel it;
  • the probe keeps inspect/inspect_err, per the repository's style.