[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 execution client asks to override the builder. 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, in batches of 500, to every configured node at once, on spawned tasks with signing off the async runtime. One send of each kind runs at a time; a send that finds the last still running skips. 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 proposal guard and the signing path are unchanged. 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 node is recorded with the block's slot and root, as Teku does, and a publish for any other block is refused. 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, a timeout, or a publish that outlives the proposal budget 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 refused connection is a definite failure, counted as a failed proposal even when it arrives after the budget, by the publish task itself
Metrics	ethlambda_validator_builder_registrations_sent_total, ..._builder_registration_failures_total, ..._blinded_blocks_proposed_total, ..._blinded_blocks_unconfirmed_total

Known limitations

• No slashing-protection record, as for every block this client signs. The in-process guard covers blinded blocks the same way.
• Per-validator builder toggles (Lighthouse's PATCH route) are out of scope; the flags apply to the whole process.
• A registration whose send fails is not resent until the next epoch's full send, even though it is cached right away.

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.