[MegaRedHand]

Fourth step of MEV-Boost builder support. With this PR, a fulu proposer registered through register_validator can receive a builder's blinded block from produceBlockV3 and publish it back through blinded_blocks. Stacked on #696.

Scope: the beacon node only. The validator client does not yet register, ask for blinded blocks or publish them; that is the next PR. Gloas (the builder API alongside gossip bids) comes after.

What it adds

Piece	Behavior
shouldOverrideBuilder	Read from engine_getPayloadV5, carried on the built payload
Registration cache	Each validator keeps the latest registration it forwarded to the builder. An entry lasts 2 epochs after it was last sent, and an older timestamp never replaces a newer one
Circuit breaker	Keeps the builder out while the node is syncing or the head is optimistic. Also trips on more than --builder-fallback-skips (3) consecutive empty slots, on more than --builder-fallback-skips-per-epoch (8) empty slots in the last 32, or on more than --builder-fallback-epochs-since-finalization (3) epochs since the state's finalized checkpoint
Bid validation	Checks, in order: nonzero value, parent hash, prev_randao, timestamp, block number, withdrawals root, blob count and blob gas, gas limit against the registered target, then the BLS signature (DOMAIN_APPLICATION_BUILDER). Every refusal is counted by reason
Selection	Uses builder_boost_factor. The local payload wins iff local >= factor * bid / 100. A tie goes local; shouldOverrideBuilder beats everything; u64::MAX means builder; the default factor is 100. The winner is chosen before assembly, so only one state transition runs. If the chosen block fails to assemble, the other one is used. This includes a local choice falling back to the bid, which Lighthouse would fail instead, following beacon-APIs' "prefer local unless it is not viable"
produceBlockV3	A builder win answers the blinded block with Eth-Execution-Payload-Blinded: true and the bid's value. A local win answers as before
POST /eth/v2/beacon/blinded_blocks	Fulu, SSZ or JSON. Checks the parent, the proposer and its signature, claims the slot, then submits to the builder on a spawned task. The builder's 202 becomes 200 and its 400 is passed through; anything else is a 500. There is no local fallback, since that would be a second block
Slot guard	post_block, the gloas post_block and blinded_blocks all claim the slot after the signature check and before anything leaves the node. A different root for a claimed slot gets a 400, and so does a slot below the finalized slot
Metrics	lean_beacon_builder_bid_rejections_total{reason}, lean_beacon_builder_payload_source_total{source}, lean_beacon_builder_fallback_total{reason}

Known limitations

• The slot guard is in memory and per node. A restart forgets it, and two beacon nodes behind one validator client do not share it. It stops this node from handing out two different blocks for a slot, which is the case the builder flow adds. It is not slashing protection.
• Registered proposers wait for the bid. The header request runs alongside the local build and is bounded by --builder-header-timeout (1 s), so it adds at most the difference.
• Block assembly still runs on the async runtime, as the local path already did. Moving it onto a blocking task is a follow-up.
• Not checked, as in every client reviewed:
  • the bid's fee recipient (relays pay with a transaction; see docs/spec_deviations.md);
  • its execution requests;
  • the header fields only an execution client could verify.

Tests

• Bid validation: one case per check.
• Selection: a table of local and bid outcomes.
• Circuit breaker: every trip, plus edges near genesis and across the block_roots wrap.
• Registration cache.
• produceBlockV3 against the mock relay:
  • builder win;
  • local win;
  • override;
  • boost factor;
  • relay down;
  • invalid bid;
  • both sides failing.
• blinded_blocks:
  • accepted;
  • builder 400;
  • builder 500;
  • the claim surviving a failed submit;
  • the cross-endpoint guard;
  • the body limit.
• CLI flags.

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which implements the Fulu builder flow (MEV-Boost integration) for ethlambda. Let me analyze the key components systematically.

Overall Assessment

This is a substantial, well-structured PR adding builder/MEV-Boost support. The code is generally high quality with good test coverage, but I found several correctness and safety issues that need attention.

---

Critical Issues

1. Integer Overflow in choice.rs Boost Calculation (Line 58-62)

let boosted = u128::from(boost_factor).saturating_mul(bid / 100);

File: crates/net/rpc/src/beacon/builder_flow/choice.rs

Problem: The bid / 100 division before multiplication loses precision for small bids. A bid of 99 wei becomes 0, making any boost_factor > 0 result in boosted = 0, causing incorrect local payload selection.

Fix: Use proper fixed-point arithmetic:

let boosted = u128::from(boost_factor)
    .saturating_mul(bid)
    .saturating_div(100);

This preserves precision and matches the documented semantics: "local payload wins iff local >= factor * (bid / 100)" — actually the docs say factor * bid / 100, not factor * (bid / 100).

---

2. Missing Pruning in PublishedProposals — Memory Growth

File: crates/net/rpc/src/beacon/blinded_blocks.rs, lines 88-98

pub(crate) fn claim(
    &self,
    slot: Slot,
    root: Root,
    finalized_slot: Slot,
) -> Result<(), ClaimError> {
    if slot < finalized_slot {
        return Err(ClaimError::Finalized);
    }
    let mut published = self.0.lock().expect("published proposals lock poisoned");
    *published = published.split_off(&finalized_slot);
    // ...
}

Problem: split_off(&finalized_slot) keeps entries >= finalized_slot, but the check slot < finalized_slot only prevents new claims below finalized. However, if finalized_slot advances past existing entries, those entries are pruned. But: the split_off uses &finalized_slot as key — this is correct for BTreeMap (keeps key >= given).

Actually, re-reading: this looks correct. The issue is more subtle — there's no background pruning. If the node never produces blocks, old entries accumulate. But given the bounded nature (only when this node proposes), this is acceptable.

Status: Acceptable for now, but consider documenting this is bounded by proposal frequency.

---

3. Race Condition in Block Production — Double Proposal Risk

File: crates/net/rpc/src/beacon/proposal.rs, lines 608-700

The produce function fetches bid and builds payload concurrently:

let (built, el_version, bid) = tokio::join!(
    build_payload(store, engine, fee_recipients, &state, head_root, proposer),
    graffiti::execution_client_version(engine, own_version),
    fetch_bid(request.as_ref(), slot),
);

Then later claims the slot after building:

// In post_block / post_blinded_block, not in produce!
if let Err(err) = published.claim(slot, root, finalized_slot(&store)) {

Problem: The slot claim happens in the publish endpoint, not in produce. This creates a window where:

1. produce returns a blinded block to validator client
2. Validator client signs and submits to post_blinded_block
3. Meanwhile, another request (or retry) could produce a different block for same slot

Actually, looking more carefully: produce doesn't claim. The claim is in post_blinded_block and post_block. But produce can be called multiple times before publication. This is by design — the validator client might request, then not publish. But it means the node could produce multiple different blocks for the same slot if called repeatedly.

The actual risk: The PublishedProposals claim in post_blinded_block checks signed.message.hash_tree_root(). But what if two different produce calls return different blinded blocks (e.g., different bids arrived), the validator signs both, and submits both? The second would be caught by PublishedProposals if roots differ.

This appears correct — the claim prevents equivocation at publish time. The window between produce and publish is the validator client's responsibility.

---

4. BLS Signature Verification Missing Domain Check in verify_proposer_signature

File: crates/blockchain/state_transition/src/beacon/stf/mod.rs, lines 224-235

pub fn verify_proposer_signature(
    state: &BeaconState,
    proposer_index: ValidatorIndex,
    block_root: Root,
    signature: &BlsSignature,
) -> bool {
    let Ok(proposer) = state.validator(proposer_index) else {
        return false;
    };
    let domain = get_domain(state, constants::DOMAIN_BEACON_PROPOSER, None);
    let signing_root = compute_signing_root(block_root, domain);
    bls::verify(&proposer.pubkey, signing_root, signature)
}

Problem: The fork parameter to get_domain is None, which uses the state's current fork. For a blinded block, this is correct if the state is at the block's slot. But this function is public and documented as working for "a blinded block has the root of its full block, so one check serves both."

Verification needed: Confirm that advance_to_slot in the caller advances through any epoch boundaries, which would update the state's fork. Looking at post_blinded_block:

let state = advance_to_slot(&parent_state, slot, &config)
    .map_err(|_| "advancing the parent state failed")?;

This should handle fork transitions. Status: Likely correct, but worth explicit verification in tests.

---

5. Builder Bid Validation — Missing Execution Payload Hash Check

File: crates/net/rpc/src/beacon/builder_flow/bid.rs, lines 89-118

The validate function checks many header fields but not block_hash. The comment says:

What is left unchecked (the payload's execution validity, its block hash and the fields only an execution client could verify) is trusted to the relay

Problem: The block_hash is commit-revealed later — the proposer signs the blinded block, then the builder reveals the full payload. If the builder lies about block_hash, the revealed payload won't match, but the proposer is already committed.

Status: This is the fundamental trust model of MEV-Boost. Documented and accepted. But consider adding a comment that this is intentional and matches other clients.

---

6. Critical: should_override_builder Not Used in Local Block Path

File: crates/net/rpc/src/beacon/proposal.rs, lines 650-670

let local_offer = local.as_ref().ok().map(|parts| LocalOffer {
    value: wei_u128(&parts.built.block_value),
    should_override_builder: parts.built.should_override_builder,
});

This is used in choice::choose. But looking at local_block function, the should_override_builder field from BuiltPayload is only read for the choice, not passed through to the actual block.

Wait — re-reading: should_override_builder is an input to the choice, not a field in the produced block. The execution client is saying "prefer local regardless of bid value." This is correct usage.

But: Is should_override_builder persisted anywhere? If the execution client sets this flag, does the block production honor it in the block itself? Looking at BuiltPayload:

pub should_override_builder: bool,

This comes from the engine API. It's only used in the choice, not in the block. This is correct — it's a hint for payload selection, not a consensus field.

---

7. Potential Division by Zero in choice.rs

File: crates/net/rpc/src/beacon/builder_flow/choice.rs, line 58

let boosted = u128::from(boost_factor).saturating_mul(bid / 100);

If bid < 100, bid / 100 == 0. With boost_factor > 0, boosted == 0. Any positive local.value wins, which may be unintended.

Actually this matches the documented "factor * (bid / 100)" but the docs also say "100 compares the two as they are." With bid=50, boosted = 0, so local wins with any positive value. This seems like a precision loss bug.

Fix: Use the corrected formula from Item 1:

let boosted = u128::from(boost_factor)
    .saturating_mul(bid)
    .saturating_div(100);

---

8. Missing Check: builder_boost_factor Upper Bound

File: crates/net/rpc/src/beacon/proposal.rs, line 375

#[serde(default)]
builder_boost_factor: Option<u64>,

No validation that this is a reasonable percentage. u64::MAX is explicitly handled as "always take builder" (except override), but what about values like 10_000 (10000%)?

Status: The math uses u128 for intermediate, so no overflow. Very large values just heavily weight toward local. This is acceptable — it's a client configuration.

---

9. Test Gap: No Test for should_override_builder in SSZ Serialization

File: crates/net/engine/src/building.rs, lines 264-280

The GetPayloadV5Response deserializes should_override_builder with #[serde(default)]. Tests verify JSON deserialization, but no test verifies SSZ encoding/decoding if BuiltPayload is ever SSZ-encoded.

Looking at BuiltPayload — it's not derived SszEncode/SszDecode. It's only used internally. Status: Acceptable.

---

10. Lock Poisoning Handling — Inconsistent Panic Strategy

File: Multiple files

The code uses .expect("... lock poisoned") throughout, which panics on poisoned locks. Examples:

• crates/net/rpc/src/beacon/blinded_blocks.rs:93
• crates/net/rpc/src/beacon/builder_flow/cache.rs:52
• crates/net/rpc/src/beacon/proposal.rs:564

Problem: In a consensus client, a panic is a DoS vector. A poisoned lock from a panicked thread brings down the whole node.

Recommendation: Consider using parking_lot locks which don't poison, or handle poisoning gracefully. This is a broader codebase issue, not specific to this PR.

---

Medium Issues

11. Builder Request Doesn't Include builder_boost_factor in Cache Key

File: crates/net/rpc/src/beacon/proposal.rs, fetch_bid function

The builder bid is fetched without considering builder_boost_factor. If the same slot is requested with different factors, the same bid is returned. But produce is called fresh each time, so this is fine — no caching at this layer.

Status: Correct, no issue.

---

12. advance_to_slot Error Handling in post_blinded_block

File: crates/net/rpc/src/beacon/blinded_blocks.rs, lines 183-186

let state = advance_to_slot(&parent_state, slot, &config)
    .map_err(|_| "advancing the parent state failed")?;

The error is discarded. advance_to_slot can fail for various reasons (invalid state transition, etc.). Logging the actual error would help debugging.

Fix:

.map_err(|err| {
    error!(%err, "advancing the parent state failed");
    "advancing the parent state failed"
})?;

---

13. Missing Timeout on submit_blinded_block

File: crates/net/rpc/src/beacon/blinded_blocks.rs, lines 220-235

let submitted = tokio::spawn(async move {
    let result = client.submit_blinded_block(&signed, ForkName::Fulu).await;
    // ...
})
.await;

The client.submit_blinded_block likely has its own timeout (from BuilderClient config), but this isn't obvious. If not, this could hang indefinitely.

Check: Verify BuilderClient has a timeout configured. Looking at initialization in main.rs, the builder_header_timeout is used. But is there a separate timeout for submit_blinded_block?

Looking at BuilderClient — not shown in diff, but the builder_header_timeout is specifically for getHeader. The submission timeout might be different.

Status: Needs verification. Consider adding explicit timeout documentation.

---

14. Registration Timestamp Comparison — Potential Replay

File: crates/net/rpc/src/beacon/builder_flow/cache.rs, lines 45-48

Some(entry) if registration.timestamp < entry.registration.timestamp => {}
Some(entry) => {
    entry.registration = registration.clone();
    entry.seen = epoch;
}

An older timestamp is ignored, but equal timestamps replace. If a validator client resends the same registration with the same timestamp, it refreshes the lifetime. This is correct.

But: what if a malicious relay sends old registrations with forged timestamps? The timestamp field is not cryptographically verified — it's just a u64 in the ValidatorRegistrationV1.

Status: This matches the trust model — the builder API client trusts the relay. The validator client is expected to use current timestamps.

---

Minor Issues / Suggestions

15. Typo in Comment

File: crates/net/rpc/src/beacon/builder_flow/bid.rs, line 274

/// The gloas gossip predicate this reuses agrees with Lighthouse's

Should be "gas" not "gloas" (which is a fork name).

---

16. Inconsistent Error Type for post_blinded_block

File: crates/net/rpc/src/beacon/blinded_blocks.rs, line 153

return ApiError::Internal("no builder is configured (--builder)").into_response();

This returns 500 for a configuration issue. A 400 or 503 might be more appropriate since the endpoint requires a builder to be configured.

Actually, looking at beacon-APIs spec — this is a server configuration issue, so 500 is arguably correct. But 503 "Service Unavailable" with a clear message might be better.

Status: Stylistic choice, not a bug.

---

17. Test Naming Inconsistency

File: crates/net/rpc/src/beacon/proposal.rs, line 1420

async fn a_bid_worth_more_than_the_local_payload_is_served_blinded() {

Tests use a_... prefix inconsistently. Some use the_.... Not a functional issue.

---

18. Documentation Clarification Needed

File: docs/rpc.md, lines 452-453

A bid is checked before it competes: a non-zero value, the head's execution block as its parent, the proposal's prev_randao and timestamp, the parent's block number plus one, the withdrawals root, the blob commitment and blob gas bounds, a gas limit within one step of the parent's toward the registered target, and the builder's signature.

Missing: the fee_recipient is explicitly not checked, as noted in spec_deviations.md. The RPC docs should mention this or link to the deviation.

---

Correctness Verification: Fork Choice and Consensus

19. Circuit Breaker Empty Slot Detection

File: crates/net/rpc/src/beacon/builder_flow/breaker.rs, lines 108-118

fn is_empty(state: &BeaconState, i: Slot) -> bool {
    match (
        get_block_root_at_slot(state, i),
        get_block_root_at_slot(state, i - 1),
    ) {
        (Ok(root), Ok(previous)) => root == previous,
        _ => false,
    }
}

Verification: This checks if block_roots[i] == block_roots[i-1]. In process_slots, empty slots carry forward the previous block root. This is correct per the comment.

Edge case: At slot 0, i - 1 would underflow. But is_empty is only called with i >= 1 (see check: (1..slot).rev() and window_start = slot.saturating_sub(SLOTS_PER_EPOCH).max(1)). Verified correct.

---

20. State Transition for Blinded Block

File: crates/net/rpc/src/beacon/proposal.rs, blinded_block function

The assemble_blinded_block is called with BlindedBlockInputs. This must:

1. Process the block through the state transition
2. Compute the state root
3. Return a BlindedBeaconBlock (with ExecutionPayloadHeader, not full payload)

Not shown in diff, but assuming this exists and is correct. The test a_bid_whose_block_fails_to_build_falls_back_to_a_local_block_that_fails_too verifies fallback behavior.

---

Summary

Severity	Count	Items
Critical	1	Item 1: Integer overflow/precision loss in boost calculation
High	1	Item 7: Division by zero-like behavior in choice (same root cause)
Medium	3	Items 12, 13, 14
Low/Style	5	Items 15, 16, 17, 18, 10

Required Changes

1. Fix choice.rs line 58 — use saturating_mul(bid).saturating_div(100) instead of saturating_mul(bid / 100)
2. Add error logging in post_blinded_block for advance_to_slot failure
3. Fix typo "gloas" → "gas" in bid.rs comment
4. Verify/document submit_blinded_block timeout behavior
5. Add cross-reference in rpc.md to spec_deviations.md for fee recipient

Good Practices Observed

• Comprehensive test coverage with property-based style tests
• Clear separation of concerns (bid validation, breaker, choice, cache)
• Proper use of tokio::spawn for builder submission to prevent client hangup cancellation
• PublishedProposals prevents equivocation across full and blinded block paths
• Metrics for

---

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

[github-actions]

🤖 Codex Code Review

Overall this looks solid. The new builder flow is thoughtfully guarded: bid validation is fairly strict, the anti-equivocation slot claim is shared across both publish paths, and the circuit-breaker logic is explicit and tested.

• No correctness or security bug jumped out in fork choice / justification / attestation / STF / SSZ areas from this PR’s touched code; the risky surface here is mostly builder integration, and the checks are generally conservative.

• One thing I’d flag is a potential availability footgun in crates/net/rpc/src/beacon/blinded_blocks.rs:177 and crates/net/rpc/src/beacon/blinded_blocks.rs:199. The slot is claimed before the builder submission result is known, and the claim is never released if submit_blinded_block returns 400/500 or the task fails. That means a transient builder failure can permanently block a retry of the same slot through this node, even though nothing was published. If that behavior is intentional, it should be documented as “signing commits the slot locally”; otherwise consider claiming only after a successful handoff, or rolling the claim back on failure.

• Related to that, crates/net/rpc/src/beacon/proposal.rs:258 claims the slot before publish_beacon_block, and crates/net/rpc/src/beacon/proposal.rs:267 returns 500 if P2P is down without releasing the claim. Same operational consequence: a local transport failure can make retried publication impossible for that slot. For slashing protection this may be acceptable, but it is stricter than “published once” and closer to “attempted once”.

• In crates/net/rpc/src/beacon/builder_flow/choice.rs:52, the boost comparison computes factor * (bid / 100) rather than (factor * bid) / 100. That floors early and slightly favors the builder/local boundary for bids not divisible by 100 wei. Probably negligible in practice, but if you want exact beacon-APIs semantics, reorder to multiply first with saturating arithmetic and divide afterward.

• In crates/net/rpc/src/beacon/builder_flow/breaker.rs:90 and crates/net/rpc/src/beacon/builder_flow/breaker.rs:95, both skip counters scan historical slots linearly on every proposal. That’s fine at epoch scale, but the consecutive-skip scan can walk up to slot-1 until the first filled slot. It’s not dangerous, just worth noting if proposal hot paths become tighter; bounding it by the threshold would avoid needless reads once tripping is inevitable.

• Minor maintainability point: crates/net/rpc/src/beacon/registrations.rs:100 records cached registrations before the async forward completes. That matches the local-fallback design, but it means block production may consult registrations the builder never actually accepted. If that is the intended trust model, a short comment near builder.registrations.record(...) is enough; otherwise cache-on-success would be safer.

If you want, I can turn these into ready-to-post GitHub review comments.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 704: builder flow for fulu block production

This covers the builder bid fetch, bid validation, the circuit breaker, the local-vs-builder choice, POST /eth/v2/beacon/blinded_blocks and the equivocation guard. Overall it is carefully built and well tested. I found no correctness or security blockers. I read the diff but did not build it or run the tests. The notes below are minor.

What looks good

• Fallback ordering in produce: the chosen source is built first, and the other is tried only if that build fails. local_error surfaces only if the builder does not step in, so a bad bid or a failed local build never costs the proposal.
• Bid checks: they run cheapest first, with the BLS pairing last. The state transition is also a backstop through assemble_blinded_block, and a bid that fails it is counted as state_transition.
• Equivocation guard:
  • PublishedProposals is shared by the full-block, gloas and blinded publish paths.
  • Retrying the same root is allowed.
  • Claims are made only after proposer, parent and signature checks, so an unauthenticated caller can't poison a slot.
  • The slot == finalized_slot boundary is tested.
• Blinded publish: the submission runs in a detached tokio::spawn, so a client disconnect can't cancel the reveal after the proposer has signed. There is deliberately no local fallback after signing.
• Circuit breaker: the "empty slot iff block_roots[i] == block_roots[i-1]" logic is correct. Slot 0 is never judged, and slots older than the state's history count as filled. The wrap-around and genesis-edge tests cover this.
• Arithmetic: checked_add on block number, saturating_mul in choose, and the u64::MAX and should_override_builder special cases are handled explicitly.

Minor points

1. choose truncates the bid (choice.rs, bid / 100 before multiplying). With factor 100, a bid of 1 ETH + 99 wei loses to a local payload of exactly 1 ETH, because the bid is rounded down to a multiple of 100 wei. The impact is negligible. If you want it exact, use local * 100 >= factor * bid with saturating_mul or a u128 overflow guard. The current form is defensible since it matches the beacon-APIs text.

2. The bid fetch could be skipped (proposal.rs, produce). fetch_bid runs in the tokio::join! even when builder_boost_factor == 0 (the local payload always wins). The builder request is wasted and its latency, up to the header timeout, still gates the response. Skipping the request in builder_request when the factor is 0 would help. should_override_builder is only known after the build, so that case can't be skipped up front.

3. PublishedProposals is in-memory only. After a restart, a validator client that retries with a different block for a slot already signed gets no protection. This is consistent with the repo's note that there is no slashing-protection record, but the spec_deviations doc could say it explicitly.

4. Blocking work in post_blinded_block. store.get_state(&parent_root) is a storage read that runs on the async task. The state advance and BLS check are correctly in spawn_blocking. Moving the state read inside it would match the pattern in registrations.rs. The endpoint also advances a full state per request with no concurrency limit. That is acceptable on a validator-client-facing API, but worth knowing.

5. Fee-recipient mismatch is only logged at debug! (accepted_bid). That matches the documented relay behavior. A counter would help operators spot a relay that stops paying through a payload transaction.

6. Small simplification: Packed::droppable() rebuilds empty_sync_aggregate() for every comparison. It only runs on error paths, so this is cosmetic.

Tests
Coverage is good: the selection table, one rejection per bid field, forged and wrong-domain signatures, the breaker boundaries, registration expiry, and the CLI default and requires behavior. A test for the boost_factor == 0 fetch skip would go with point 2 if you adopt it.

---

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