[MegaRedHand]

Fifth step of MEV-Boost builder support: the validator client side. With --builder-proposals, the client registers its validators with the beacon node's builder and accepts a blinded block when the builder's bid wins. It signs the blinded block and publishes it back to the node that produced it. Stacked on #704.

Scope:

• Covered: fulu and earlier (electra blinded blocks are accepted for third-party beacon nodes).
• Gloas is unchanged: produceBlockV4 keeps its self-build configuration, and registrations stop at the gloas fork. The gloas builder API comes in phase B.

Flags

Flag	Behavior
--builder-proposals	Off by default. Registers validators and sends builder_boost_factor with every pre-gloas block request
--builder-boost-factor <u64>	Default 100. Requires --builder-proposals
--prefer-builder-proposals	Sends u64::MAX, so the builder wins whenever there is a valid bid, unless the node's execution client sets shouldOverrideBuilder. Requires --builder-proposals; conflicts with --builder-boost-factor

Without --builder-proposals, nothing changes: the client sends builder_boost_factor=0 and refuses a blinded answer, as before.

What it adds

Piece	Behavior
Registrations	One ValidatorRegistrationV1 per validator the node resolved, signed under the builder domain (genesis fork version, zero validators root). The signature and timestamp are cached until the validator's fee recipient or gas limit changes, and a re-signed one is always newer than the last (max(now, last + 1)). The full set is sent after each epoch refresh and changed keys every slot, on spawned tasks with signing off the async runtime, in batches of 500 to every configured node, each node working through its own batches beside the others; a node that cannot be reached is sent no more batches that round. One send runs at a time, and each reads the cache only once the previous one was answered, so an older copy does not arrive after a newer one: a per-slot send that finds another running skips and a later slot sends the change, a full send waits for a per-slot one, and a full send that finds the last full one still running skips its epoch. A per-slot send no node accepts is sent again, as signed, every slot until a node takes it or the next full send ends the retry. Validators with no fee recipient are skipped with one warning, and a zero --suggested-fee-recipient is reported at error at startup
Blinded answer	ProducedBlock gains a blinded variant. Its root is the full block's, so the signing path is unchanged, and its slot goes into the durable EIP-3076 slashing-protection record (#734) before it is signed, like a full block's. Slot and proposer are checked as for a full block. The fee-recipient check is skipped, since the header names the builder's address
Blinded publish	POST /eth/v2/beacon/blinded_blocks, SSZ, only on the node that produced the block, with no failover: only that node's builder holds the payload, and only that node claimed the slot. The produced block carries its producer (Producer, filled in by the fallback), and the proposal path sends the signed block back to that node. A blinded answer is refused before signing when builder proposals are off. The request runs on a spawned task, so the proposal budget cannot cut it off
Unconfirmed publish	A 5xx other than 503, or no answer once connected, is reported as "unconfirmed", not as a missed proposal: the relay may still publish the block. Nothing retries, since the slot is signed and a second block would be an equivocation. A 4xx, a 503 or a connection never made (refused, or past the client's 3 s connect timeout) is a definite failure, counted as a failed proposal. The budget is enforced inside ProposalService::propose_by, which returns a Proposed outcome: past the budget a signed blinded block comes back as StillPublishing, and whichever of the duty loop and the publish task ends up holding the outcome (decided by a oneshot channel) reports it
Metrics	ethlambda_validator_builder_registrations_sent_total, ..._builder_registration_failures_total, ..._blinded_blocks_proposed_total, ..._blinded_blocks_unconfirmed_total

Known limitations

• Per-validator builder toggles (Lighthouse's PATCH route) are out of scope; the flags apply to the whole process.
• A full registration send that fails is not retried until the next epoch's. Only a per-slot send, which carries a keymanager change, is retried every slot until then.

Tests

• Unit: the blinded decode and its root, the HTTP query and status mapping, producer-only publish in the fallback, the registration signature domain, the cache and batching, the gloas stop, the proposal path's outcomes, and the flags.
• End to end through ethlambda's own beacon node and the mock relay:
  • registration → blinded produce → sign → publish → relay;
  • a full block when builder proposals are off;
  • a full block when the relay is down.

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR for the builder proposals feature in ethlambda. This is a substantial change adding MEV-Boost style builder block proposals to the validator client.

Security & Correctness

Critical: Blinded block publish isolation is correct but fragile

crates/validator/src/beacon_node/fallback.rs:294-317 — The publish_blinded_block implementation correctly pins blinded publishes to the producing node. However, the blinded_producer lock uses unwrap_or_else(std::sync::PoisonError::into_inner) which recovers from poisoned locks by clearing the error. This is dangerous: if a thread panicked while holding the lock, the Option<(Slot, usize)> could be in an inconsistent state. Given this is consensus-critical (prevents equivocation), consider whether panicking is safer.

Critical: u64::MAX as "prefer builder" sentinel

bin/ethlambda/src/validator.rs:95-100 — Using u64::MAX as the boost factor for --prefer-builder-proposals is correct per the spec, but the arithmetic local >= factor * bid / 100 could overflow if implemented naively on the node side. The PR doesn't show the node-side implementation, but verify that multiplication is checked or uses uint256.

Medium: Race between register_changed and register_all

crates/validator/src/registration.rs:335-341 — register_all stores signed_generation after current_blocking completes, but register_changed reads it before checking. If register_changed fires during register_all's current_blocking, it will see stale generation, re-sign everything, and send duplicates. The register_changed spawn in lib.rs:304 happens after register_all completes, so this is unlikely in practice, but the ordering is subtle.

Performance

Good: Chunked signing off async runtime

crates/validator/src/registration.rs:280-302 — SIGN_CHUNK = 64 with spawn_blocking properly avoids blocking the async runtime. The per-chunk store read lock is correct.

Concern: register_changed every slot even with no changes

crates/validator/src/lib.rs:368-370 — The spawn happens unconditionally off the slot loop. The generation check is fast (atomic read), but this still allocates a task per slot. Consider whether this is necessary when builder is None or when no validators are registered.

Rust Idioms & Code Quality

Good pattern: then_some for conditional construction

bin/ethlambda/src/validator.rs:93 — self.builder_proposals.then_some(...) is idiomatic.

Issue: std::sync::Mutex in async code

crates/validator/src/beacon_node/fallback.rs:46 — blinded_producer: std::sync::Mutex<Option<(Slot, usize)>> is held across await points in produce_block (line 137) and publish_blinded_block (line 311). This is technically fine since the critical sections are brief, but tokio::sync::Mutex would be more idiomatic for async contexts. The std::sync::Mutex is used elsewhere in this file for consistency with existing code.

Issue: unwrap_or_else(PoisonError::into_inner) pattern repeated

Multiple files (fallback.rs, proposal.rs, registration.rs) use this pattern. Consider a helper or parking_lot which doesn't poison.

Error Handling

Good: BlindedUnconfirmed distinction

crates/validator/src/error.rs:72-80 — Properly distinguishes "relay may have published" from definite failures. The comment correctly notes this is never retried.

Issue: task.await.unwrap_or_else in publish_blinded

crates/validator/src/proposal.rs:589-593 — If the spawned task panics, this returns Ok(BuilderUnconfirmed), which is correct (unknown state), but the panic info is lost. Consider logging the panic payload if available.

Consensus & Spec Compliance

Good: Fork-aware blinded block handling

crates/validator/src/beacon_node/block_contents.rs:348-369 — Explicitly rejects blinded blocks for forks that don't support them (Phase0 through Deneb, Gloas, Lean). The exhaustive match forces future fork additions to make a decision.

Question: Gloas builder market transition

crates/validator/src/registration.rs:257-260 — Registrations stop at Gloas, but the PR description says "from gloas the builder market works through proposer preferences instead." Is there a corresponding proposer_preferences implementation, or is this a TODO? The produceBlockV4 path in http.rs doesn't show preference handling.

Testing

Good: Comprehensive test coverage

The validator_client_tests.rs builder flow module tests end-to-end: registration, blinded production, publish, and failure modes. The fallback.rs tests verify no failover on blinded publish.

Missing test: Clock observation for blinded publish delay

crates/validator/src/proposal.rs:550-558 — The publication delay metric is only observed when slot_start is Some. No test verifies this path with a real SlotClock.

Documentation

Minor: Typo in CLAUDE.md

CLAUDE.md:855 — "pre-gloas slots register validators with the node's builder and may sign a blinded block, published to the producing node only" — "published" should be "publishable" or rephrase for clarity.

Minor: docs/cli.md inconsistency

docs/cli.md:296 — --builder-boost-factor default is 100, but bin/ethlambda/src/validator.rs:84 shows default_value = "100". The description says "0 prefers the local payload unless it cannot be built" but the CLI help says "0 prefers the local payload unless it cannot be built" — verify this matches node behavior.

Suggestions

1. Line 137 of fallback.rs: Consider tokio::sync::Mutex for blinded_producer to avoid blocking the async runtime, or document why std::sync::Mutex is sufficient (hold time is brief).

2. Line 589 of proposal.rs: Log the panic payload if the publish task panicked, not just the JoinError.

3. Line 335-341 of registration.rs: Move signed_generation.store before self.send to reduce the window for the race with register_changed, or document why the current ordering is safe.

4. General: The PoisonError::into_inner recovery pattern appears 10+ times. Consider whether parking_lot::Mutex (no poisoning) would simplify and be safer.

Summary

The implementation is correct and well-tested for the core security property: blinded blocks don't fail over to wrong nodes. The ProposalGuard integration prevents double-signing. The main concerns are around lock poisoning recovery in consensus-critical paths and a subtle race in registration caching. The code is idiomatic Rust with good async patterns for the signing workload.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The validator-side builder flow is careful about proposer-safety: the slot is recorded at signing time, blinded publishes are isolated onto a background task, and the timeout path no longer misclassifies a slow relay as a missed proposal.

• No correctness or security bug jumped out in the changed paths around crates/validator/src/proposal.rs:515 and crates/validator/src/lib.rs:677.
• The most important consensus property here—never re-signing a second block for the same slot even if publication overruns—is preserved by the guard plus signed_blinded_at, and the new tests cover that well in crates/validator/src/proposal.rs:978 and crates/validator/src/proposal.rs:889.

Minor nits only:

• crates/validator/src/proposal.rs:544 logs Published::Imported / Published::BuilderUnconfirmed as unexpected for blinded publish, but still increments success counters for any non-BuilderUnconfirmed outcome at crates/validator/src/proposal.rs:552. If Imported is truly impossible on this path, consider treating it as an error instead of counting it as a successful blinded proposal; otherwise metrics can hide a beacon-node/API contract regression.
• crates/validator/src/lib.rs:683 / crates/validator/src/lib.rs:692 key off signed_blinded_at(slot) after the proposal future resolves. That is fine with the current one-duty-per-slot model, but it couples loop behavior to shared mutable state. A slightly cleaner pattern would be to return an explicit “signed blinded” marker from ProposalService::propose so the caller does not have to re-infer path selection from side state.

If you want, I can also do a second pass on the beacon-node / RPC-side builder changes from the same PR for spec conformance and edge cases.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 705 (validator client: builder proposals and blinded publish)

I read the diff through the end of the HTTP client changes, including the CLI flags, the fallback node, http.rs, block_contents.rs, the rpc test refactor and the end-to-end tests. I did not read the rest of the diff, which likely holds the validator-service wiring, registration signing and caching, and metrics. I did not build the code or run the tests. The review below covers what I read.

The design is sound. The main safety properties hold:

• No failover on blinded publish. publish_blinded_block goes only to the producing node, recorded by (slot, position) in FallbackBeaconNode. A second node cannot be handed a signed block it never produced.
• No double-sign. Nothing retries after the slot is signed.
• Stale claims are cleared. The record is overwritten on every successful produce_block, so a full block clears an earlier blinded claim.
• Unconfirmed vs. definite failures. The split in publish_blinded_block (crates/validator/src/beacon_node/http.rs) is sensible. Connect errors and 503 are definite because the node cannot have acted on the block. Other 5xx errors, timeouts and resets are unconfirmed.
• Blinded answers are only decoded when requested. A blinded answer is refused when builder_boost_factor is None, so a client with builder proposals off never signs a block it cannot publish.
• Gloas and the other forks are refused by name. from_blinded_ssz has no _ arm, so a new fork forces a decision.
• CLI rules are tested. The requires and conflicts_with rules are exercised by parsing real argument lists.

Minor points and questions

1. 503 as "definite" (http.rs, publish_blinded_block). This holds for ethlambda and Lighthouse, where 503 means syncing before the builder is touched. A reverse proxy or load balancer can also return 503 after forwarding the request, and then the relay may still publish. The doc comment says "the node cannot have acted", which is only true for that assumption. Consider saying so in the doc, or treating a 503 as definite only when the body or headers match a sync refusal. The cost of the current choice is only a mislabelled metric, since nothing retries either way.

2. Fallback record with produce_block failure (fallback.rs, produce_block). If every node fails, the early return via ? leaves the previous record in place. This is harmless because publish checks the slot. Writing None on failure as well would make the invariant "record reflects the latest produce" literal, but I do not think it is needed.

3. register_validators timeout on the fallback path. REGISTRATION_TIMEOUT is 12 s per node, and try_all is sequential. With N unreachable nodes a registration round can take up to N × 12 s. The PR says this runs on spawned tasks, so it does not block duties. Please confirm that no per-slot "changed keys" send can queue behind a previous round indefinitely.

4. Unused parameter. publish_blinded_block in http.rs takes _slot, which only the fallback layer uses. That is fine for the trait, but a one-line comment would stop someone from "fixing" it.

5. Copy of the body. post_ssz_body calls body.to_vec(). A blinded block is small, so this does not matter. For full blocks with blobs it was already the existing behaviour, and the refactor does not make it worse.

6. Boost factor semantics. The doc on --builder-boost-factor says the local payload wins iff local >= factor * bid / 100. With u64::MAX the node side must use widened arithmetic or special-case the value. That is in the prior PR, but it is worth confirming that feat(rpc): serve and publish builder blocks (fulu) #704 handles it without overflow.

Tests. Coverage is good. It includes status mapping, the connect-failure case, fallback routing and no-failover, a registration JSON round trip, and end-to-end tests against the real router with a mock relay. Moving the builder test world into proposal::test_support is a clean reuse.

Overall: LGTM on what I read. The points above are minor, with Point 1 the most worth a doc tweak.

---

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

[pablodeymo]

Security read of this PR, with the signing side and the publish/network side reviewed separately. No critical or high issues: the slashable path is unchanged apart from which container wraps it, the per-validator guard is recorded before sign_block with no await between, a blinded block goes only to the recorded producer with no failover and fails closed on any mismatch, an unconfirmed publish is never retried, and the registration domain is the genesis-version builder domain with a zero root (and the tests pin it apart from the chain and envelope domains). The node-side diff is test-only. One thing I'd fix before merging, plus smaller ones.

1. Bound the epoch registration send (lib.rs, registration.rs::send, fallback.rs::try_all)

register_all is spawned every epoch with no in-flight check, send walks batches sequentially, and try_all walks nodes sequentially with a 12 s timeout each. 10k validators are 20 batches, so one hung node costs 4 min per epoch; two hung nodes exceed the epoch, the next task starts while the previous is still running, and each one holds the full registration set and re-sends it to the healthy nodes. A relay that is slow but alive (the node waits up to 10 s on it per batch) makes this the normal case rather than a failure case. A single-flight guard on the epoch send, and fanning batches out per node with a concurrency cap, would bound it. Duties are not affected either way, since both sends are off the slot loop.

2. Make the registration timestamp strictly newer per key

current stamps a re-signed registration with now_secs and caches it before the relay answers. A backward NTP step between two keymanager changes, or two changes within one second, produces a timestamp not newer than the one the relay holds; the relay drops it, the VC re-sends the cached one every epoch, and the builder keeps the old fee recipient or gas limit silently. timestamp = max(now, last_signed + 1) per key closes it.

3. Refuse the zero address in --suggested-fee-recipient

The CLI parser checks hex and length only, while the keymanager refuses the zero address. With the zero default, every registration is signed naming the burn address and the relay pays it. Pre-existing for prepare_beacon_proposer, but this PR turns it into a signed, relay-persisted commitment.

Smaller

• ProposalService::propose relies on HttpBeaconNode alone to refuse a blinded answer when builder proposals are off. One check before signing (blinded && self.builder_boost_factor.is_none()) would match the file's own "last point before a wrong answer becomes a signature" policy, plus a test with a hand-built Contents::Blinded.
• The producer record in FallbackBeaconNode is keyed by slot only and read inside the publish task, not bound to the signed body. Safe today because the node list is immutable and propose runs sequentially behind the guard; recording (slot, block_root, position) and checking the root at publish would make the "only the producing node" property checked rather than assumed.
• The reqwest client keeps the default redirect policy, so a 307 from the producing node's front proxy re-POSTs the signed blinded block to whatever Location names. redirect::Policy::none() would make "only the producing node" hold at the HTTP layer too. The produce body and the error bodies are also read uncapped and reflected into error-level logs with newlines intact; a small cap and truncation would do.
• Classification edges: ethlambda's own 500s from blinded_blocks ("no builder is configured", "checking the blinded block failed") are definite non-publishes but read as unconfirmed; a proxy's 503 after the upstream received the request is read as definite. Neither leads to a retry or a second signature; worth a doc line.
• A 400 on a batch marks all 500 entries failed and keeps the other 499 off the relay until next epoch; split-and-retry on 400 would localise a bad key.
• Worth a line in spec_deviations.md: the "signed but undelivered" window for a blinded block is up to 8 s versus sub-second for a full block, and the process has no signal handling, so a SIGINT mid-publish aborts the detached task; also that last_blinded_slot is process-global because one slot has one proposer.

Tests I'd add: a blinded block refused by propose with builder proposals off; the signature inside the published SignedBlindedBeaconBlock verifying under the full block's root; a second propose refused after the dropped-future path; wrong slot, proposer or fork on a blinded answer; timestamp monotonicity across a backward clock step; a timeout or reset after send on the publish path; an overlapping epoch send against a hung node; and an end-to-end case through FallbackBeaconNode and ProposalService for relay-refuses, relay-times-out, and the node's claim refusing a second block.

Checked and clean: guard keyed per pubkey and strictly advancing, checked before produce_block and recorded before sign_block; last_blinded_slot set before the first post-sign await; the spawned publish survives the budget drop and every outcome arm only logs and counts; publish_blinded_block indexes the recorded node only and refuses any other slot, and a later full block clears the record; the signed root is the decoded container's, equal to the full block's, under the requested slot's proposer domain, with slot, proposer and fork pinned to the request; blinded decode accepts electra and fulu only; blocking signing tasks capture only Arcs and pubkeys, nothing new derives Debug on a secret, logs carry pubkeys and slots only; 0 sent and a blinded answer refused when the flag is off; both registration entry points stop at the slot's fork.

[MegaRedHand]

/review

[github-actions]

🤖 Codex Code Review

No actionable findings. The blinded-block SSZ handling, signing domain, proposal guard, and publication path look consistent. No consensus-transition or fork-choice changes were introduced.

Tests could not run because Rust toolchain initialization attempted to write to the read-only rustup directory.

---

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

[github-actions]

🤖 Claude Code Review

Review: validator-client builder proposals (PR 705)

Overall this is solid, well-documented work. The equivocation-sensitive parts are handled correctly:

• The guard is recorded before signing.
• A blinded publish is never retried and never fails over.
• The publish runs on a spawned task, so dropping the proposal future at the budget cannot cancel an in-flight signed block.
• The registration signing domain (compute_builder_domain(genesis_fork_version), zero validators root) matches builder-specs, and tests pin it.

The clap requires/conflicts_with tests are a nice touch. The issues below are mostly edge cases.

Correctness

1. Registration timestamps can collide (registration.rs, current()).

• A changed registration is re-signed with timestamp: now_secs.
• If an operator changes a fee recipient or gas limit twice within the same second, both registrations carry the same timestamp.
• Relays generally keep a registration only if its timestamp is strictly newer, so the second change can be silently dropped.
• The cached registration still says the new values, so nothing re-sends it until a later change.
• Suggested fix:

  let timestamp = signed.get(pubkey)
      .map_or(now_secs, |held| now_secs.max(held.message.timestamp + 1));

2. ProposerSettings::generation only moves in the per-key write path and forget (proposer_settings.rs).

• Confirm no other setter changes what fee_recipient(pubkey) or gas_limit(pubkey) returns. Examples would be a default fee-recipient or default gas-limit setter, or keystore import paths.
• If one exists and doesn't bump the counter, register_changed short-circuits and the change waits for the next epoch's register_all.
• A test asserting "every mutator bumps generation" would lock this down.

3. refresh_epoch now returns Vec<BlsPubkey> (lib.rs ~L742).

• The syncing branch (if beacon_node.is_optimistic_or_syncing().await? {) isn't shown in the diff.
• If it returns Ok(Vec::new()) rather than an Err, the caller sets last_epoch and spawns register_all with an empty list.
• That retain wipes the whole signed cache and known.
• The result is that every validator is re-signed (thousands of BLS ops) on the next epoch, and no per-slot changes are sent in between.
• Please double-check that path returns an error, or skip register_all when resolved is empty.

4. signed_blinded_at(slot) is keyed on slot only (lib.rs propose, ~L690–L718).

• Any Ok(Err(_)) for a slot whose blinded block was signed is swallowed.
• If propose were ever re-entered for the same slot, the guard's ProposalRefused would vanish silently.
• The module docs say not to retry, so this is fine today.
• Consider having publish_blinded own the outcome explicitly instead, e.g. return a distinct marker or clear last_blinded_slot after the task reports.

Design / robustness

5. Implicit coupling through FallbackBeaconNode::blinded_producer (fallback.rs).

• Publish correctness depends on produce→publish being strictly sequential, with no other produce_block in between, because a full-block produce clears the record.
• That holds today since there is one proposer per slot and a sequential duty loop. It is hidden shared state, though.
• Carrying the producing node's position inside ProducedBlock (or a returned token) would make the invariant structural rather than temporal.

6. 503 treated as a definite refusal (http.rs publish_blinded_block).

• The doc claims a 503 means the node refused before contacting the builder. That holds for an ethlambda node but is not guaranteed for third-party beacon nodes, some of which surface builder/relay failures as 503.
• Impact is limited to metrics and logs, since neither path retries.
• Worth either a comment noting it is an ethlambda-specific assumption, or classifying 503 as unconfirmed too, to err toward "may be published."

7. Fee-recipient check fully skipped for blinded blocks (proposal.rs).

• The reasoning is correct.
• Still, a debug! that includes the header's fee_recipient would make post-hoc auditing ("which builder paid?") possible without extra tooling. Currently only a generic message is logged.

Nits

• bin/ethlambda/src/validator.rs, --prefer-builder-proposals help (also in docs/cli.md): "unless the execution client asks to override the builder (sends builder_boost_factor = u64::MAX)" reads as if the EL sends u64::MAX. Suggest: "Sends builder_boost_factor = u64::MAX, so the builder's block is taken whenever there is a valid bid, unless the execution client asks to override the builder."
• http.rs post_ssz_body: body.to_vec() copies. It was pre-existing for full blocks (up to several hundred KiB with blobs). Taking Vec<u8> or bytes::Bytes by value would avoid the copy now that this is a shared helper.
• registration.rs current(): the signed std mutex is held across all BLS signing in a chunk. That is harmless on the blocking pool, but it serializes a concurrent register_all and register_changed. Fine at SIGN_CHUNK = 64; just noting it.
• register_all registers every resolved validator, including exited/withdrawn ones. Filtering by status would save signatures and relay load on large, long-lived key sets.

Nothing blocking. Item 1 and Item 3 are the ones I'd address before merge.

---

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

[github-actions]

🤖 Gemini Code Review

This PR is well-structured, thorough, and cleanly handles the subtle failure modes of the builder flow (routing blinded blocks strictly to the producing node, distinguishing definite refusals from BlindedUnconfirmed outcomes, and keeping the blinded publish alive on its own task if it outlives the slot's proposal budget).

One concurrency edge case worth considering in RegistrationService:

1. crates/validator/src/registration.rs (lines 226–295) & crates/validator/src/lib.rs (lines 317–321, 381–383): register_all and register_changed can run concurrently
   In lib.rs, register_all is spawned at the start of an epoch's first slot and register_changed is spawned at the end of that same slot. Because neither method serializes against the other:

   • If settings.generation() was bumped before or during register_all (while register_all is still in current_blocking or awaiting HTTP batches in self.send), signed_generation has not yet been updated to match, so register_changed will start a concurrent pass.
   • If a keymanager update lands after register_all has signed chunk 0 (putting the old registration into register_all's current.all), register_changed can sign and send the new registration while register_all is still mid-send, and register_all may then send the stale registration from its current.all afterwards (and overwrite signed_generation with the older generation it read at line 237).

   Serializing the slow paths of register_all and register_changed behind a tokio::sync::Mutex<()> (acquired in register_changed only after the fast-path generation == self.signed_generation.load(Ordering::Acquire) check fails, and re-checking generation once acquired) would preserve the single-atomic-load fast path while preventing register_all and register_changed from interleaving.

---

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

[github-actions]

🤖 Grok Code Review

The builder flow is carefully split between definite refusal and unconfirmed publish, and keeping blinded publishes off the failover path is the right anti-equivocation choice. A few correctness gaps are still worth fixing.

1. A failed fee-recipient update is not retried until the next epoch. In registration.rs, both register_all and register_changed insert into the signed cache and store signed_generation before send. On send failure the module docs say the change waits for the next epoch's full send. Until then the relay still has the previous registration, so builder blocks keep paying the old address (or the old gas limit) for up to an epoch. That is the case a keymanager update is meant to fix within a slot. Do not advance signed_generation (and do not treat the cache as authoritative) unless the batch was accepted, so the next slot retries. A full-set failure can keep waiting for the epoch; a changed fee recipient should not.

2. Cached registration timestamps never move. current() reuses the original signed message, timestamp included, until fee recipient or gas limit changes. Resubmitting that blob does not refresh age if a relay rejects now - timestamp above its max (often hours to a few days). A long-running process with stable settings will silently stop winning builder bids. Re-sign when the cached timestamp is older than a few hours, not only when settings change. The comment that "repeating them" handles relay expiry is only true if expiry is last-seen, not the signed timestamp.

3. blinded_producer is a single slot and is read only when the detached publish is polled (fallback.rs, produce_block / publish_blinded_block). publish_blinded is spawned so it can outlive the proposal budget, and the next slot's produce_block overwrites that Option. If the task is not polled before that overwrite, the lookup misses and a block that was already signed is never sent (InconsistentResponse), or, if another node produced a blinded block for the same slot, it can be handed to a builder that never saw the bid — the failure mode this type is written to prevent. Capture the node index at produce time (or keep a small slot → index map) and pass that into the publish task instead of looking it up later.

4. --prefer-builder-proposals help text is wrong (bin/ethlambda/src/validator.rs around the flag, and the same sentence in docs/cli.md). This flag sends builder_boost_factor = u64::MAX to prefer the builder. The execution client does not send that value; its override is shouldOverrideBuilder, which prefers local. As written, the parenthetical describes the opposite signal. Also, u64::MAX * bid / 100 overflows u64 for any real bid. This client relies on the beacon node special-casing u64::MAX. That is tested against ethlambda; a third-party node that implements the documented formula with wrapping mul will prefer local instead. Worth stating in the flag docs.

5. 202 on blinded_blocks is counted as handed to the builder. publish_blinded_block maps 202 to BroadcastNotImported, but publish_blinded then still increments blinded_blocks_proposed_total ("handed to its builder") for every outcome except BuilderUnconfirmed. For a blinded block, 202 often means validation failed before the relay accepted it. Do not count that as a builder handoff.

The unconfirmed-vs-4xx split, the proposal-guard interaction, builder-domain tests, and the no-failover tests are in good shape. Item 1 is the one that can pay the wrong address; Item 3 is the one that can drop or misroute an already-signed block.

---

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

[github-actions]

🤖 DeepSeek Code Review

Overall the builder flow is well-structured and the blinded-block signing/root handling looks correct. No consensus-state transitions changed. Main concerns:

1. Registration timestamp reuse may violate builder expectations
   In crates/validator/src/registration.rs, current() reuses the cached SignedValidatorRegistrationV1 unchanged whenever fee recipient and gas limit match. The periodic full send therefore can retransmit a registration with an old timestamp for many epochs. Builder relays commonly expect timestamp to be fresh/current; old timestamps may be rejected or treated as stale. The module docs claim this is harmless, but that is worth validating against target relays; if interop is a goal, re-sign with a fresh timestamp on the full epoch send.

2. Overlapping register_all and register_changed can race
   register_all and register_changed are both spawned independently in crates/validator/src/lib.rs, and they can overlap for large validator sets. Both methods call current_blocking() and then write signed_generation with Ordering::Release. A slower register_all can overwrite a newer generation stored by register_changed, and it can also send a stale cached registration after a newer changed registration has already been sent. This could regress a builder’s fee recipient/gas limit. Serialize these registration paths, or at minimum use AtomicU64::fetch_max for generation and avoid re-sending stale full batches after a change.

3. Partial signing failures are not retried until the next epoch
   In crates/validator/src/registration.rs, current_blocking() logs and skips a whole SIGN_CHUNK if spawn_blocking returns Err. register_all() still stores signed_generation afterward, so validators in that failed chunk are not retried by register_changed and remain unregistered until the next epoch’s full send. If the failure is transient, this delays registration unnecessarily. Consider not advancing signed_generation on partial failure, or retrying failed chunks within the same epoch.

4. Removing a fee recipient does not unregister the previous builder registration
   current() skips validators with no fee recipient, but it does not send anything that tells the builder to forget the last registration. If a validator was previously registered and the operator later removes the fee recipient, the old registration can remain active at the builder/relay, still directing payments to the previous address until it expires or is overwritten. This should be documented explicitly and probably warned on the path where a previously registered key becomes None.

Minor: Error::BlindedUnconfirmed omits status from its Display output despite carrying Option<u16>; including it would make logs more actionable.

---

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

[github-actions]

🤖 Kimi Code Review

1. Blinded-publish routing can be overwritten while a publish is still pending — crates/validator/src/beacon_node/fallback.rs:264-327. blinded_producer stores only the latest (Slot, usize), and every subsequent production overwrites or clears it. Because blinded publication is deliberately detached and may outlive its proposal budget, a later produce_block can run before the spawned publish reads this record. A different slot then causes a spurious “no beacon node produced” failure; another blinded production for the same slot could route the body to the wrong node. Keep per-in-flight records until their publish completes, removing only the matching record; keying by slot plus proposer/block root would also cover multiple duties. Add a test where another block is produced before a delayed blinded publish starts.

2. Epoch and changed-registration sends can arrive out of order — crates/validator/src/registration.rs:263-353, with both entry points spawned independently in crates/validator/src/lib.rs. register_all and register_changed have no serialization around signing and sending. An older full send can therefore reach the node/relay after a newer fee-recipient or gas-limit update. This is particularly ambiguous when both calls sign within the same second, since registrations then have identical timestamps and timestamp-based relay ordering cannot reliably determine the newest. It can leave the builder using stale settings until another epoch, or longer if equal/old timestamps are rejected. Use a single registration worker or serialize the operations, and make re-signed timestamps strictly increasing per validator, e.g. max(now, previous_timestamp + 1). The generation counter alone does not prevent stale network sends, although it generally ensures a later retry.

3. The blinded-publish error contract disagrees with its HTTP implementation — crates/validator/src/beacon_node/mod.rs versus http.rs:748-805. The trait says a 5xx or transport failure is BlindedUnconfirmed, while HttpBeaconNode treats 503 and connection failures as definite errors. Those exceptions are reasonable and documented on the implementation, but the shared contract should explicitly include them so other implementations and callers classify outcomes consistently.

Otherwise, the builder-domain signing, pre-gloas fork gating, no-retry/no-equivocation behavior, and detached-publish metrics handling look carefully implemented, with good test coverage.

---

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

[MegaRedHand]

Thanks for the reviews. The changes are in ee5fed8, e919655, 7eced44 and 25f3ca5, with #704's latest merged in at 9823ed7. I checked each point against Lighthouse, Prysm, Teku, Lodestar and Nimbus, so nothing here goes against what they do.

@pablodeymo's security review

1. Bound the epoch registration send: done, two ways.

   • One send of each kind at a time. The epoch's full send skips, with a warning, while the previous one is still running. The per-slot send of changed keys skips silently and picks the change up next slot. Teku skips a busy round the same way (registrationInProgress). Lighthouse, Lodestar and Nimbus await each round, so theirs cannot overlap either.
   • Every node at once. The calls that must reach every node, registrations among them, now go to all nodes concurrently instead of one after another. Batches stay sequential, as in Lighthouse, which broadcasts each 500-entry batch to all nodes with join_all. A hung node now costs each batch one timeout, not one per node, and it no longer holds back the healthy nodes behind it.

2. Strictly newer timestamp per key: done. A re-signed registration gets max(now, last + 1). No other client does this; all of them use the raw clock. It only affects what we sign, though, and relays accept it either way. Without it, the cached signature would keep a timestamp the relay drops on every epoch's re-send.

3. Zero address in --suggested-fee-recipient: warned, not refused. No client refuses it on the command line:

   • Lighthouse, Teku, Lodestar and Nimbus accept it.
   • Prysm warns ("Fee recipient is burn address") and registers it anyway.
   • Lodestar's default is the zero address.

   Only Teku's keymanager refuses it, as ours already does (http_api/proposer.rs). The client now logs an error at startup when the default is the zero address, saying that it burns rewards and that registrations sign it for the builder.

Smaller points:

• A blinded block when builder proposals are off: propose now refuses it before signing, on top of the HTTP client's refusal, with a test using a mock that breaks the contract. Of the other clients, only Lodestar refuses before signing; the rest sign it.

• The producer record: bound to the block. It is now (slot, block_root, position), and a publish for any other root is refused. This is exactly Teku's blockCreatorCache, keyed by slot and block root, which is the one other client that pins a blinded block to its producer. A production that fails on every node now clears the record too.

• Redirects: kept as reqwest's default. None of Lighthouse, Prysm, Teku or Lodestar sets a redirect policy on the validator client's connection to its beacon node. Nimbus's HTTP library isn't in my checkout, so I couldn't check it. Unlike the relay in feat(beacon): --builder and register_validator #696, the beacon node URL is the operator's own, and so is any proxy in front of it.

• Error bodies: done. A failed answer's body is read up to 1 MiB, which is enough for a pool submission's per-entry failure list. At most 1 KiB of it goes into the error, with every control character written out (line breaks, escape sequences, U+2028 and U+2029). No client caps these. Lighthouse escapes newlines only by accident, through Debug.

• Classification edges: documented, in publish_blinded_block's doc and in spec_deviations.md. Neither edge leads to a retry or a second signature. For comparison, no other client separates "definitely not published" from "maybe published" at all.

• Splitting a batch the node refuses with a 400: not done. No client splits:

  • Lighthouse re-sends the whole batch the next slot.
  • Teku retries the round after 30 s.
  • Prysm, Lodestar and Nimbus wait for the next round.

  Every entry is signed here by keys this client holds, so a 400 usually points at the node or the relay rather than at one entry. The module doc says so.

• spec_deviations.md: now covers:

  • the signed-but-undelivered window (up to 5 s at the node plus 8 s at the client);
  • the missing signal handling;
  • the producer record bound to the root, compared with Teku;
  • signed_blinded_at being one per process.

Tests: all added:

• blinded with builder proposals off, refused before signing (the second attempt is refused by the same check, not the guard);
• the signature in the published SignedBlindedBeaconBlock verifying over the full block's root;
• a second propose refused after the dropped-future path;
• wrong slot, proposer and fork on a blinded answer;
• timestamps across a same-second change and a backward clock step;
• an overlapping epoch send against a slow node;
• concurrent fan-out;
• a publish for the wrong root;
• a failed production clearing the record;
• error bodies on one line and capped.

End to end through ethlambda's node and the mock relay, I added three cases:

• relay refuses: a definite 400 at the client;
• relay answers 500: BlindedUnconfirmed;
• after a blinded block is handed over, the node refuses a different full block for the slot.

They go through HttpBeaconNode rather than ProposalService. The test world's state isn't pinned to the schedule's fulu fork version, so the service's own signature would not verify there. The service's paths for these outcomes are covered with the mock. I used a 500 for the "relay times out" case: the node's submit timeout is 5 s, and the node maps a timeout to the same 500.

Automated reviews

• u64::MAX overflow (Kimi, Claude): the node scales in u128 and saturates, as of feat(rpc): serve and publish builder blocks (fulu) #704's (factor * bid) // 100 change.
• register_changed racing register_all (Kimi): each now runs single-flight. When the two overlap, the cache's mutex means a key one of them signed is reused by the other, never signed twice. At worst a changed key also goes out in the full send, which re-sends every key anyway.
• std::sync::Mutex and poison recovery (Kimi): kept, like the rest of the crate. No lock is held across an await, and every write is a single map operation.
• The panic payload of the publish task (Kimi): JoinError's Display already carries the panic message, and that is what is logged.
• Imported counted on the blinded path (Codex): neither implementation returns it there. It is logged as an outcome that belongs to another call. I kept counting it, because the block did go out.
• An explicit "signed blinded" marker from propose (Codex): when the budget drops the proposal, propose never returns, so the duty loop has nothing to read a marker from. That is why the state lives on the service.
• 503 as definite (Claude): documented, as above.
• The record after a failed produce_block (Claude): cleared now.
• N × 12 s with unreachable nodes (Claude): the nodes are now called concurrently, and each send is single-flight.
• The _slot parameter (Claude): commented.

[pablodeymo]

Reviewed by reading the diff against feat/beacon-builder-blocks at c09478b; I did not build or run it. I found no equivocation or signing-safety problem: the blinded block goes through check_and_record_block like a full one. Three behaviour points first, then smaller ones. Paths are under crates/validator/src/ unless they say otherwise.

Worth a look

1. A stale registration can overwrite a fresh one (registration.rs:308). register_all and register_changed run under separate in-flight guards, and register_all sends a snapshot in sequential 500-key batches. A later batch can therefore deliver a pre-change registration after register_changed already sent the new one. The node stores the fee recipient of every registration it is sent (crates/net/rpc/src/beacon/registrations.rs:159), so the old address wins until the next epoch. It needs more than 500 validators and a slow node.
2. A publish that never left the client is reported as unconfirmed (beacon_node/http.rs:820). The client sets a total timeout (8s) and no connect_timeout, and only is_connect() is treated as definite. With the producing node unreachable, the total timeout fires during connect, so a slot that is certainly missed is logged as "the relay may still publish" and lands on the unconfirmed counter. ClientBuilder::connect_timeout would make it a connect error.
3. One hung node throttles registrations to every node (registration.rs:358). send awaits each batch across all nodes before starting the next, so a node that hangs costs REGISTRATION_TIMEOUT (12s) per batch. With two nodes, one hung, and 5,000 validators, the healthy node gets its last batch about two minutes into the epoch.

Smaller

4. beacon_node/http.rs:538: the partial-failure path of handle_pool_submission logs each failure.message raw, so the new one-line, capped error_text does not cover it.
5. proposal.rs:1093: a_blinded_publish_survives_a_dropped_proposal assumes the proposal reaches signing within a 50 ms wall-clock timeout. The slashing-protection write and the signatures sit before that point, so it can flake on a loaded runner.
6. registration.rs:291: register_all takes the std mutex signed on an async worker, while current() holds it on a blocking thread across a whole 64-key signing chunk, so the worker can stall for that long.
7. beacon_node/mod.rs:412: the trait contract says any 5xx or transport failure is BlindedUnconfirmed, but the HTTP implementation returns a definite error for a 503 and for a connect failure, while the mock follows the doc. The doc and the implementation should state one rule.
8. beacon_node/http.rs:94: error_text and read_error_body re-implement error_text and read_body from crates/net/rpc/src/beacon/builder_client/mod.rs. This copy escapes the whole body (up to 1 MiB) before cutting it to the limit, and can cut an escape sequence in half.
9. registration.rs:359: send uses a match whose arms are both side-effect only; CLAUDE.md asks for inspect / inspect_err there.

Design, take or leave

10. beacon_node/mod.rs:173: Published::BuilderUnconfirmed lives in the enum a node returns, though only ProposalService builds it, so every consumer carries arms for a state a node cannot answer.
11. lib.rs:717: the duty loop reads a proposal's outcome through signed_blinded_at, shared state set inside propose, in three guard arms. The outcome could carry that fact.
12. beacon_node/fallback.rs:46: which node produced a blinded block is kept as single-slot state in the fallback. Returning the node's position with the ProducedBlock and passing it back to publish would remove the record and the mismatch path.

Description

"No slashing-protection record, as for every block this client signs" is out of date since #734: the blinded block is recorded in the EIP-3076 record before it is signed.

[MegaRedHand]

Thanks @pablodeymo. Every point and the description are addressed in ad59936 (points 1 to 9) and 2e4ec29 (10 to 12).

Worth a look

1. A stale registration overwriting a fresh one: fixed. Both kinds of send now share one lock (RegistrationService::sending). A send takes it before it reads the cache and holds it until its last batch is answered, so each node receives registrations in the order the cache had them.
   • A per-slot send that finds the lock taken skips. Its change is still unsent, so a later slot picks it up: the first after the full send ends, which against a node slow to answer each batch can be several slots. docs/cli.md's keymanager rows now say "usually the next slot".
   • A full send waits for a running per-slot send instead of skipping its epoch.
   • A full send that finds the previous one still running or waiting skips, as before.
   • New tests: a_change_is_never_overtaken_by_a_full_send_that_read_before_it (your scenario) and a_full_send_waits_for_a_running_per_slot_send.
   • The node side (registrations.rs) still records every registration's fee recipient, as Lighthouse does. One window is left: a request the client timed out on that the node still processes afterwards. The next epoch's full send closes it.
2. An unreachable producing node counted as unconfirmed: fixed. The client now sets connect_timeout (3 s, inside the 8 s total). reqwest 0.12 reports a connect-phase timeout as both is_connect() and is_timeout(), and the connect check comes first, so it is a definite failure.
   • a_blinded_publish_whose_connection_times_out_is_a_definite_failure covers it. It points an https URL at a listener that accepts TCP but never answers the TLS handshake, so no unroutable address is needed.
   • With the connect_timeout line removed, that test fails after 8 s with BlindedUnconfirmed, which is the behaviour you described.
3. One hung node throttling every node: fixed. Batching moved below the fallback's fan-out, into a new trait method, register_validators_in_batches. Each node now works through its own batches alongside the others.
   • A node that cannot be reached is sent no more batches that round, so a hung node costs one timeout rather than one per batch.
   • A batch counts as sent when any node accepted it, so both counters keep their documented meaning.
   • Tests: a_hung_node_holds_back_only_its_own_batches and an_unreachable_node_is_sent_no_more_batches.

Smaller

4. Each partial-failure failure.message now goes through error_text.
5. a_blinded_publish_survives_a_dropped_proposal no longer relies on wall-clock timing. The mock now holds the publish behind a semaphore and notifies when the call arrives, and the test drops the proposal on that notification (the deadline itself is now tested separately; see 11). The registration concurrency tests and every_node_is_called_at_once use the same kind of gate, and the mock's delay hook is gone.
6. With the send lock, the signed cache is only ever taken under it, so register_all's retain never waits on a signing chunk.
7. There is now one rule, the implementation's: a 4xx, a 503 or a connection never made is definite; any other 5xx, or no answer once connected, is unconfirmed.
   • The trait contract, Error::BlindedUnconfirmed, BlindedOutcome::Unconfirmed, publish_blinded, metrics.md and spec_deviations.md all state it.
   • The mock's guard failure is now definite (an unreachable node).
8. error_text now lives in one place, ethlambda_types::text::one_line. Both the builder client (feat(beacon): --builder and register_validator #696's builder_client/mod.rs) and the validator client call it.
   • It walks the text once, stops at the cap, and never cuts a character or an escape in two.
   • The validator copy no longer adds a ... suffix.
   • read_error_body stays separate from read_body, since the two treat an over-long body differently: the first keeps the head of it, the second refuses it.
9. send now uses inspect/inspect_err.

Design

All three are taken.

10. BuilderUnconfirmed is out of Published. Published again holds only what a node can answer. propose returns its own outcome instead, Proposed::{Full(Published), Blinded(BlindedOutcome)}, where BlindedOutcome is Published(Published) | Unconfirmed | StillPublishing.
11. signed_blinded_at is gone. The budget moved into ProposalService::propose_by(slot, duty, store, deadline):
    • Production, signing and a full block's publish end at the deadline with Error::ProposalDeadline.
    • A signed blinded block's publish is not cut off. Past the deadline, propose_by returns Blinded(StillPublishing).
    • The publish task hands its outcome back over a oneshot. If the duty loop is still waiting, the loop reports it. If not, the send fails and the task reports it. On the deadline, close() + try_recv() takes an outcome that was sent at that moment. For a caller that awaits propose_by to the end, as the duty loop does, each outcome is reported once. The publish_blinded doc names the two edges this does not cover: a task that panics after the deadline, and a caller that drops the proposal after the task has sent its outcome.
    • The duty loop's guarded arms became plain matches on Proposed. propose(slot, duty, store) stays as the no-deadline form.
    • Tests: a_proposal_within_its_deadline_returns_what_the_node_said, a_blinded_refusal_within_the_deadline_is_the_proposals_error, a_blinded_publish_past_the_deadline_still_goes_out and a_proposal_past_its_deadline_before_signing_is_abandoned. The last two pause and advance the clock once the mock holds the call, so neither depends on timing.
12. The producer travels with the block. ProducedBlock gains a Producer. The fallback fills it in at produce_block, and publish_blinded_block(producer, fork, body) goes back to that node.
    • The record, its mutex and the mismatch path are gone, as are the HTTP client's unused slot/block_root parameters.
    • So are the three tests that existed only for the record (wrong slot or root, cleared by a failed production, cleared by a full block).
    • The proposal path takes the producer from the same ProducedBlock it signed, so the pairing the record used to check at runtime holds there without a check. A position the fallback never handed out is still refused.

Description

The slashing-protection line is fixed: the blinded block's slot goes into the durable EIP-3076 record before signing. The "Registrations", "Blinded publish" and "Unconfirmed publish" rows now describe the per-node batches, the send lock, the connect timeout, the producer token and the deadline.

[MegaRedHand]

/review

[github-actions]

🤖 Codex Code Review

No actionable correctness or security findings. The blinded-block flow preserves durable slashing protection, validates slot/proposer/fork before signing, and publishes back to the producing node.

Tests could not run: Rustup attempted to update the configured toolchain but its temporary directory is read-only.

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 705, validator-client builder proposals

This PR is careful and well documented. I found no consensus-safety bugs. Two properties hold:

• Slashing protection and signing: a blinded block's root is the full block's root, so protection is recorded before the signature and the signature is over the correct root. A test verifies this with BLS.
• No equivocation path: blinded publishes never fail over and never retry, and the deadline handling cannot drop a signed block.

The notes below are mostly about edges and small hardening.

Correctness

1. lib.rs refresh_epoch: confirm what entries holds. The function now returns entries.iter().map(|entry| entry.pubkey), and register_all treats that as the complete resolved set. It calls signed.retain(...) and sets known = pubkeys. The doc comment says "Resolve any newly activated validators". If entries is only the newly resolved delta and not every resolved key, each epoch would wipe the cache and known down to that delta. Registrations for everyone else would then stop.

   • Please check this.
   • Add a test that runs two consecutive epochs with no new activations and asserts the full set is still registered.

2. registration.rs register_changed/register_all can lose a change for an epoch. current_blocking logs and skips a chunk whose spawn_blocking fails, but signed_generation is still advanced to generation.

   • A keymanager change for a key in the failed chunk is then invisible to register_changed until the next full send.
   • A panic there is unlikely, but the fix is cheap: advance signed_generation only when every chunk succeeded.

3. http.rs publish_blinded_block treats every 503 as definite. The e2e test (a_relay_refusing_the_blinded_block_is_a_definite_refusal) shows the ethlambda node passes the relay's status through. If it also passes through a relay's 503 (relays do return 503 under load, possibly after publishing), that case is logged as a missed proposal.

   • The documented consequence is cosmetic: no retry happens.
   • Still, consider having the node map relay 5xx to 500 or 502 so 503 keeps the meaning "the node refused before forwarding". At minimum, add that edge to the list in the doc comment.

4. registration.rs: a changed registration is cached even when its send fails. This is documented. The effect is that a fee-recipient change through the keymanager can stay unsent to the builder for up to an epoch, while blinded blocks keep paying the old address. Two cheap options:

   • Keep signed_generation unadvanced when every batch of a register_changed send failed, so the next slot retries.
   • Or note the up-to-an-epoch delay in the docs/cli.md keymanager table.

Security and hardening

5. Bounded error bodies. read_error_body, error_text and text::one_line are good hardening: they cap the read size and escape control and line-separator characters before logging.

   • Moving one_line into ethlambda-types puts a logging utility in the consensus-types crate. It is acceptable, but it fits better in a small util crate if one exists.

6. No fee-recipient check for blinded blocks. Skipping it matches Lighthouse and Teku, and trust is delegated to the relay. It may be worth logging the bid's header fee_recipient and the validator's registered address side by side at debug level, so an operator can reconstruct who was paid.

API and readability

7. ProposalService::new(..., Option<u64>) plus .with_clock(...). A positional Option<u64> is opaque at call sites; every test passes a bare None. Consider passing Option<BuilderProposals>, which already exists, or adding a with_builder(factor) builder method that mirrors with_clock.

8. compares_fee_recipient (proposal.rs) only wraps !produced.is_blinded(). Inlining it, with its comment, would read just as well.

9. validator.rs CLI doc for --prefer-builder-proposals. "unless the execution client asks to override the builder (sends builder_boost_factor = u64::MAX)" reads as if the execution client sends the factor. It is this client that sends u64::MAX; the override is the beacon node's or execution layer's own censorship or override heuristic. Suggested wording: "sends builder_boost_factor = 2^64-1; the beacon node may still choose the local payload if its execution client signals an override." The same wording is in docs/cli.md.

Tests

10. Coverage is thorough. It includes deadline and still-publishing races using a paused clock, dropped proposals, producer pinning across failover, batching while a node hangs, and registration ordering under the sending lock.

11. a_full_send_waits_for_a_running_per_slot_send relies on a 50 ms sleep. If the regression it guards against came back, the test could still pass on a slow machine. It will not flake into a failure, but a full_send/sending probe, as other tests use, would make it deterministic.

Minor

• Converting try_all to join_all also changes prepare_beacon_proposer, subscribe_committees and subscribe_sync_committees from serial to concurrent. This is a good change, but the PR description should mention it, since it is outside the builder flow.
• REGISTRATION_TIMEOUT (12 s) is set per request. reqwest's RequestBuilder::timeout overrides the client-wide 8 s timeout, so this works as intended.

Overall this looks good to merge once Item 1 is confirmed. Items 2–4 are small hardening follow-ups.

---

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

[github-actions]

🤖 Gemini Code Review

This PR looks solid and well-tested. Key aspects verified:

• Builder registration lifecycle (crates/validator/src/registration.rs):
  • Signatures are properly computed under DOMAIN_APPLICATION_BUILDER with the genesis fork version and a zeroed genesis_validators_root, cached per validator, and only re-signed when (fee_recipient, gas_limit) changes, with strictly monotonic timestamps (newer_than) so relays never drop rapid updates or updates across backward wall-clock steps.
  • Concurrency between per-epoch full sends and per-slot delta sends is cleanly serialized via sending and InFlight, preventing stale registrations from overtaking newer keymanager updates while keeping BLS signing off the async runtime in SIGN_CHUNK batches.
  • Registrations cleanly no-op from the Gloas fork epoch onward.
• Blinded block production & publication (crates/validator/src/proposal.rs, crates/validator/src/beacon_node/):
  • Blinded blocks are rejected prior to signing (and before recording in slashing protection) when --builder-proposals is disabled.
  • FallbackBeaconNode pins publish_blinded_block to the Producer node that returned the blinded block without failing over to nodes whose builders do not hold the payload.
  • Once a blinded block is signed, its publication runs on a detached task with a race-free oneshot handoff (close + try_recv) so the slot's attestation deadline does not cancel an in-flight relay submission, while accurately distinguishing definite failures (4xx, 503, connect errors/timeouts) from unconfirmed submissions (BlindedUnconfirmed).
• Log sanitization (crates/common/types/src/text.rs, crates/validator/src/beacon_node/http.rs):
  • Remote error bodies and per-item failure messages are bounded and sanitized via one_line before logging.

No issues found — LGTM!

---

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

[github-actions]

🤖 DeepSeek Code Review

Overall this is a well-structured PR with strong documentation and tests. I did not see consensus-critical changes to fork choice, attestation processing, or state transition. A few issues worth addressing:

1. crates/validator/src/beacon_node/block_contents.rs, in the new ProducedBlock::is_blinded:

pub fn is_blinded(&self) -> bool {
    matches!(self.contents, Contents::Blinded { .. })
}

This moves self.contents out of &self; Contents is not Copy. It should be:

matches!(&self.contents, Contents::Blinded { .. })

This is the most likely compile error in the PR.

2. crates/validator/src/beacon_node/http.rs, in read_error_body:

while bytes.len() < ERROR_BODY_READ_LIMIT {
    match response.chunk().await {
        Ok(Some(chunk)) => bytes.extend_from_slice(&chunk),
        _ => break,
    }
}
bytes.truncate(ERROR_BODY_READ_LIMIT);

The cap is applied only after extending bytes, so a single oversized chunk from a hostile or misbehaving beacon node/relay can cause a transient allocation far above ERROR_BODY_READ_LIMIT. Limit the copy before extending, e.g.:

let remaining = ERROR_BODY_READ_LIMIT - bytes.len();
bytes.extend_from_slice(&chunk[..chunk.len().min(remaining)]);
break;

This only bounds the local Vec; reqwest may still hand you an already-allocated large chunk, but it prevents the extra full-buffer copy/extension. Worth hardening.

3. crates/validator/src/beacon_node/fallback.rs, in the new register_validators_in_batches override:

for (batch, outcome) in outcomes.into_iter().enumerate() {
    ...
    accepted[batch] = true;
    last[batch] = ...
}

This assumes every BeaconNodeApi implementation returns exactly batches outcomes. An incorrect or third-party implementation returning more outcomes than batches would panic with an index out of bounds. Since BeaconNodeApi is public and generic, a defensive check or at least a documented invariant check would be safer before indexing into accepted/last.

No issues found in the XMSS/SSZ consensus paths; the blinded-block root equality is covered by tests and the signing-domain tests look correct.

---

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

[github-actions]

🤖 Grok Code Review

The blinded-publish path is carefully built: no failover, slashing protection before the signature, and the deadline cannot cancel a signed blinded publish. A few correctness and safety issues should be fixed before this lands.

1. Blinded blocks skip the fee-recipient check for the wrong reason. compares_fee_recipient in crates/validator/src/proposal.rs treats a mismatch as normal because “the header names the builder’s address.” In the builder spec the payload header’s fee_recipient is the validator’s registered address; the relay is required to match it. ProducedBlock::fee_recipient already reads that field, and full blocks warn and bump fee_recipient_mismatches_total. Skipping it means a buggy or hostile beacon node can return a blinded block paying someone else and this client will sign it with only a debug line. Call check_fee_recipient for blinded blocks too (warn-and-continue, same as full blocks). The mock bid in proposal::test_support::offer is built from an empty payload and is not rewritten to the registration’s 0x11… address, so the new end-to-end test would not catch this. Please also confirm the beacon node rejects a bid whose header fee recipient does not match the registration; that test currently accepts it.

2. --prefer-builder-proposals sends builder_boost_factor = u64::MAX (bin/ethlambda/src/validator.rs, ValidatorOptions::builder), while the new CLI text defines the comparison as local >= factor * bid / 100. That multiplication overflows: debug builds panic, release builds wrap and prefer the local payload — the opposite of the flag. The node must special-case 0 and u64::MAX before multiplying, as Lighthouse does. The same flag’s comment says the execution client overrides the builder by sending builder_boost_factor = u64::MAX. That is the same sentinel this flag sends, and the engine API override is shouldOverrideBuilder, not this query parameter. As written, “prefer builder” and “EL override” cannot both be true.

3. A failed registration is cached and not retried until the next epoch (registration.rs: the cache is updated inside current before send, and signed_generation is stored even when every batch fails). A single 400 drops the whole batch of 500, and this client deliberately does not split it. That is a long window with no builder bids. At least retry failed batches on the next slot until one full send succeeds, instead of treating a transport or 5xx failure as “already signed, wait an epoch.”

4. Clearing a fee recipient does not replace or revoke the registration the relay already has. current omits validators with no address, so later epoch sends never overwrite the last signed message. Relays keep that message until it expires, and with finding 1 this client will still sign a builder block for the old address with no mismatch metric. Document the expiry window, or keep re-sending until the relay can be given a new registration.

5. FallbackBeaconNode::register_validators_in_batches indexes accepted[batch] with the node’s outcome length and does not check it. The default impl returns one result per chunk, but any shorter or longer vec panics or reports AllBeaconNodesFailed("no nodes configured") for batches that were never sent. Zip against batches and treat a length mismatch as a node failure.

The publish classification (4xx/503/connect timeout definite, other 5xx unconfirmed), the sending lock so an older full send cannot land after a fee-recipient change, and InFlight so a hung epoch send cannot queue one task per epoch all look right. Log sanitization via text::one_line is the right fix.

---

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

[github-actions]

🤖 Kimi Code Review

No blocking correctness or security issues found.

The high-risk paths are handled carefully:

• Blinded blocks are rejected before signing when builder proposals are disabled, and slot, proposer, and fork validation still applies.
• Slashing protection is recorded before signing; unconfirmed blinded publishes are never retried or failed over, avoiding equivocation.
• Producer routing ensures the signed blinded block returns only to the producing node.
• Registration signatures use the correct builder domain with the genesis fork version, rather than the gloas envelope domain.
• Registration sends are serialized to prevent stale fee recipients from overtaking newer ones, while generation tracking avoids missed keymanager updates.
• Batch fan-out is concurrent across nodes without allowing one unreachable node to delay healthy nodes’ subsequent batches.
• Remote error bodies are bounded and sanitized against log injection and terminal escapes.

The remaining edge cases—ambiguous 5xx/503 publish classification, timed-out registrations later being processed, and incomplete reporting if a proposal future is dropped—are explicitly documented and do not appear to introduce unsafe signing behavior. Test coverage is particularly strong around these concurrency and failure paths.

---

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

[pablodeymo]

Re-reviewed at dffbfaa. This time I built it: the validator crate's 427 unit tests pass, and 36 concurrent runs of the proposal, registration and fallback tests showed no flake. I also checked two fixes by removing them: without connect_timeout the new test fails after 8 s with BlindedUnconfirmed, and without the per-slot try_lock the overtaking test fails.

All twelve points and the description are addressed. The deadline handling in propose_by reads correctly to me: a signed block always reaches its publish, and the oneshot decides who reports.

Two notes, neither blocking:

1. The send lock is global, so a changed registration waits behind the slowest node's full send. A node whose relay hangs answers each batch with a 500 after 10 s and is still given the rest, so 5,000 validators hold the lock for about 100 s. The module doc says "several slots"; a number there would help.
2. one_line no longer marks a cut body, so a truncated error text looks complete.

[MegaRedHand]

Thanks @pablodeymo. Both notes are addressed in 285f3ac. I also went back over every bot round on this PR (10-26, 11-05 and 11-13). The points still open are fixed in 99f899e and 6fd0fa5. #707 has this merged in.

1. The send lock's wait now has a number. The registration module doc says how long a keymanager change can wait behind a full send.
   • A node has 12 s to answer each batch. A node that takes longer is treated as unreachable and sent nothing more.
   • So the wait is at most 12 s per 500 validators.
   • Your case is an ethlambda node whose relay doesn't answer. It replies to each batch with a 500 after its own 10 s and is still sent the rest, so 5,000 validators wait about 100 s (eight slots), and two minutes at most.
2. A cut error text is marked again. one_line ends any text it cuts in ..., counted within the cap.
   • A text that fits exactly stays unmarked.
   • The cut backs up to the last whole character or escape that leaves room for the marker.
   • The node's builder client gets the marker on relay errors too.
   • The exact cut text is pinned in text.rs, the validator's http.rs tests and the builder client's tests, so removing the marker fails all three.

From the bot rounds

• A refused per-slot registration is re-sent every slot (6fd0fa5; Grok 11-05 docs: add initial roadmap #1, Claude 11-13 docs: add philosophy and design principles #4, Grok 11-13 docs: improve readme #3). Before, a keymanager change no node accepted waited for the next epoch's full send, and blinded blocks paid the old address meanwhile.
  • The refused keys are kept in unsent, and their registrations go out again every slot exactly as they were signed, so their timestamps don't move.
  • signed_generation stays behind while a key is waiting, so the per-slot check is still one atomic read.
  • Every full send ends the retry, whether or not it got through. A node that keeps refusing therefore costs a warning a slot for at most an epoch.
  • A failed full send still waits for the next epoch.
  • Lighthouse retries the next slot as well: it caches a registration only once a node accepts it.
  • Tests: a_change_no_node_accepted_is_sent_again_until_one_does, a_full_send_that_delivers_a_waiting_change_ends_its_retry and a_refused_full_send_ends_the_retry_too.
• A failed signing chunk (99f899e; DeepSeek 11-05 docs: improve readme #3, Claude 11-13 chore: initial repo setup #2): a chunk whose blocking task fails no longer advances signed_generation, so the next slot signs its keys. Only a panic in signing reaches this path, and there's no hook to inject one, so it is untested.
• The blinded block's fee recipients (6fd0fa5; Claude 11-05 feat: initial libp2p integration with req-resp status decoding #7 and 11-13 docs: update roadmap with extended steps #6): the debug line now names the header's fee recipient and the configured one.
• A cleared fee recipient (6fd0fa5; DeepSeek 11-05 docs: add philosophy and design principles #4, Grok 11-13 docs: add philosophy and design principles #4): docs/cli.md now says that a DELETE leaving a validator with no address sends nothing, so the builder keeps paying the last address it was given.
• The error-body copy (99f899e; DeepSeek 11-13 chore: initial repo setup #2): read_error_body copies only what fits under its limit out of each chunk.
• --prefer-builder-proposals' help (99f899e; Claude 11-05 and 11-13 feat: listen for gossipsub blocks #9, Grok 11-05 docs: add philosophy and design principles #4 and 11-13 chore: initial repo setup #2): it said the execution client sends builder_boost_factor = u64::MAX. This client sends it; the execution client's say is shouldOverrideBuilder, which the node still honours.

Left as they are

• refresh_epoch returning a delta, or an empty list wiping the cache (Claude 11-13 docs: add initial roadmap #1 and 11-05 docs: improve readme #3): entries is resolved from the full pubkey list every epoch, and a syncing node returns an error. An empty list means no key has an index, and forgetting them is then right.
• Every settings writer bumping the generation (Claude 11-05 chore: initial repo setup #2): every keymanager write goes through update or forget, and both bump it.
• Cached timestamps never refreshing (Grok 11-05 chore: initial repo setup #2, DeepSeek 11-05 docs: add initial roadmap #1): Lighthouse also re-sends its cached signed registration unchanged.
• Exited validators registered (Claude 11-05): Lighthouse registers every key with an index, exited ones included.
• A relay's 503 passed through (Claude 11-13 docs: improve readme #3): an ethlambda node answers every relay failure other than a 400 with a 500. A 503 from a proxy is the edge the publish doc already names.
• A 202 counted as handed to the builder (Grok 11-05 feat: load genesis network configuration and state #5): per beacon-APIs, the node broadcast the block anyway, so the relay had revealed the payload.
• matches!(self.contents, ..) moving the field (DeepSeek 11-13 docs: add initial roadmap #1): a pattern with no bindings moves nothing, and the code compiles.
• A fee-recipient check on blinded blocks (Grok 11-13 docs: add initial roadmap #1): a blinded header's fee_recipient is normally the builder's, and the proposer is paid by a transaction in the payload. Lighthouse and Teku skip the check too.
• u64::MAX overflowing on the node (Grok 11-13 chore: initial repo setup #2): choose takes the builder at u64::MAX before any multiplication, and boosted works in u128 and saturates.
• A node returning the wrong number of outcomes (DeepSeek 11-13 docs: improve readme #3, Grok 11-13 feat: load genesis network configuration and state #5): every node uses the trait's default, which returns one outcome per batch, and the fallback is the only override.
• The 50 ms sleep in a_full_send_waits_for_a_running_per_slot_send (Claude 11-13 docs: update roadmap #11): if the full send skipped, the final [2, 1, 2] batch assert would fail whatever the timing.
• Style (Claude 11-13 feat: initial libp2p integration with req-resp status decoding #7 and feat: respond to Status messages #8, the 11-05 to_vec nit): kept.