[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>	Builds a BuilderClient at startup, so a malformed URL stops the node there. A one-off GET /eth/v1/builder/status runs in the background and only warns, since the builder may come up after the node does. The user agent is the node's CLIENT_VERSION
--builder-header-timeout <ms>	Default 1000, at least 1, requires --builder
POST /eth/v1/validator/register_validator	JSON array or SSZ list. See below
lean_beacon_builder_registrations_total{outcome}	forwarded, failed, dropped; plus call="status" on lean_beacon_builder_request_seconds

register_validator

• 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'.
• 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 forward, so they hold even when the builder fails (as Lighthouse does).
• 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, as in Prysm, 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 and Lodestar);
  • a malformed body is a 400 either way.
• Trust: registrations are not BLS-verified here (Lighthouse does not either); relays verify them. The caller can set fee recipients this way, just as with prepare_beacon_proposer.

Tests

The tests run against the mock relay from #695:

• known, unknown, exited and pending validators;
• the empty-list skip;
• no builder;
• a builder 400 passed through;
• an unreachable builder;
• fee recipients surviving builder failures;
• SSZ bodies, malformed bodies, and 415;
• the CLI flags.

make lint passes, as do the rpc lib tests and the binary's tests.

[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