Skip to content

feat(beacon): builder-specs containers and the fulu blinded block's state root - #694

Open
MegaRedHand wants to merge 18 commits into
nightlyfrom
feat/beacon-builder-blinded-blocks
Open

MegaRedHand wants to merge 18 commits into
nightlyfrom
feat/beacon-builder-blinded-blocks

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

First step of MEV-Boost builder support: the containers the relay flow exchanges, and a way to compute a fulu blinded block's state root. Nothing calls the new public API yet; the next PR (the builder client and the produceBlockV3 builder path) does.

Base is nightly, not main. assemble_block as this extends it comes from #634, #646, #648, #668 and #678, which sit on separate stacks. This retargets to main once those merge.

What it adds

Piece Where
DOMAIN_APPLICATION_BUILDER (0x00000001), cross-referenced with gloas's DOMAIN_BEACON_BUILDER types/src/beacon/constants.rs
ExecutionPayloadHeader: From<&ExecutionPayload> and Deserialize types/src/beacon/containers/deneb.rs
builder-specs containers: ValidatorRegistrationV1, BuilderBid, BlindedBeaconBlock{,Body} and their signed forms (electra/fulu shape) types/src/beacon/builder.rs (new)
builder-specs' own example files, CC0, pinned at fce64468 types/tests/fixtures/builder_specs/
assemble_block split into full_body, unsigned_block and post_state, with no behavior change state_transition/src/beacon/block_production.rs
assemble_blinded_block same file

The blinded state root: a stand-in payload and two patches

The state transition takes only full payloads. Other clients either generalize their STF over the payload or keep a separate blinded STF (Grandine). assemble_blinded_block does neither:

  1. it checks header.withdrawals_root against the expected withdrawals;
  2. it runs process_block on a body whose payload is a stand-in: parent_hash, prev_randao and timestamp copied from the builder's header, the expected withdrawals, everything else empty;
  3. it patches the two post-state fields that depend on the payload itself, latest_execution_payload_header and latest_block_header.body_root.

It is fulu only. Fulu is the last fork with blinded blocks (gloas has none), so the process_block the argument rests on cannot change under it. The function's doc walks through fulu's process_block step by step.

blinded_and_full_blocks_reach_the_same_state_root is the guard. Around the same payload, the blinded block must equal BlindedBeaconBlock::from(full), which means the same state root and the same block root. Its state has a withdrawal due and its body a deposit request. Commenting out either patch makes it fail.

Trust boundary

Checked here Left to the caller (next PR) Trusted to the relay
withdrawals root, parent hash, prev_randao, timestamp, blob count response fork version, signature, value, gas limit, blob_gas_used every other header field, execution validity, execution requests

The last column matches every client reviewed: only an execution client could check those fields, and the payload is revealed only after the proposer signs.

Tests

  • The containers decode builder-specs' examples from JSON and SSZ, re-encode the SSZ byte for byte, and serialize back to the example JSON.
  • A blinded block's root equals the full block's on a fully populated block, so a reordered field fails under both presets.
  • There is one refusal test per header check, each asserting the specification's exact condition string.
  • make lint and cargo test --workspace --profile release-fast --lib --bins pass.

Also

fulu_state loses a stale #[allow(dead_code)] and its doc no longer contradicts the file: it already had a caller, and this adds a second.

Registrations and builder bids are signed under builder-specs'
DOMAIN_APPLICATION_BUILDER, an application domain outside the
specification's own. The mask's doc said this crate never uses one;
it now does, so the doc names it.
A blinded block carries the header of a payload this node never sees,
and blinding a locally built block needs the same conversion. The
header also needs Deserialize now, since bids and blinded blocks
arrive as JSON.
Real encodings to decode against, so the builder containers are checked
against what relays and other clients send rather than only against
themselves.
Only the bid is a response; the registrations and the blinded block
are request bodies, and the `value` key is OpenAPI's example wrapper.
Record the full commit, since nothing else in the repo pins it. The
header's JSON round trip moves beside the other round-trip tests.
The builder API's domain and gloas's in-protocol builder domain share
a name and nothing else. Copying an existing get_domain call for the
builder API would sign under the wrong one, and the only symptom is a
relay rejecting every signature.
What a validator client signs and its beacon node forwards to relays
through registerValidator, checked against builder-specs' example in
both encodings.
The signed offer getHeader returns: a payload header, the blob
commitments and execution requests that go into the body with it, and
the value paid to the proposer.
The block a validator signs when a relay supplies the payload. Its
root equals the full block's because a payload header's root is the
payload's, which is what lets one signature serve both; the tests pin
that, and builder-specs' example pins the encoding.
The root-equality test used a mostly empty body, and empty lists with
the same limit share a root, so swapping two of them still passed under
the minimal preset. Build it from the fully populated JSON test block
instead. The example tests now also check the JSON this node would send
a relay, not only the JSON it reads.
Registrations and bids are signed over their own roots, relays reject
an older registration rather than requiring an unchanged one, and
from_parts copies a state root its caller must recompute.
A blinded block needs the same body, framing and process_block run as
a full one, then patches the post-state before taking its root. No
behavior change.
The state transition takes full payloads only. Rather than thread a
payload abstraction through it, run it on a stand-in payload built from
the builder's header and patch the two post-state fields that depend on
the payload itself. Safe because fulu, the last fork with blinded
blocks, is final; the guard test fails on any drift.
The stand-in payload is what makes process_block's parent hash,
prev_randao, timestamp and blob checks apply to a builder's header,
and only if it copies the right fields. One test per check, plus the
withdrawals root and the fork gate, which this path checks itself.
Both block paths pass a constant execution engine, so on the blinded
path no one checks the builder's header beyond the fields the stand-in
copies. That is right for the state root but easy to mistake for bid
validation, so the doc now says the caller owns the rest.
The guard's state had no withdrawal due and its body no execution
request, so the withdrawals root was only ever checked against the
empty list and the "identical" rows of the doc's table were argued
rather than tested. Six payload fields the stand-in leaves empty were
empty in the test payload too.
They were single lines past the width the rest of the file wraps at.
The stand-in-and-patch approach is unusual (every other client threads
a payload abstraction through its STF), so the next person to touch
fulu's process_block should find the reason before the guard test does.
fulu_state's doc named a field it deliberately does not cover and a
step that has since been written. assemble_blinded_block's list of
what the caller must still check was short, and A2 codes against it.
The rest are small accuracy fixes found reviewing the branch whole.
@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Overall this looks solid: the blinded-block path is carefully documented, and the added guard test around state-root/root equivalence is exactly the right kind of consensus safety net.

  • No correctness or security issue jumped out in the core blinded-state-root approach. In particular, the explicit withdrawals_root check plus the equivalence test at crates/blockchain/state_transition/src/beacon/block_production.rs:704 and crates/blockchain/state_transition/src/beacon/block_production.rs:1488 are good protections for a consensus-sensitive shortcut.
  • The SSZ/JSON builder-spec tests also look useful and targeted; crates/common/types/src/beacon/builder.rs:318 and crates/common/types/src/beacon/builder.rs:340 give good coverage for wire compatibility and root preservation.

One thing I’d still consider tightening:

  • assemble_blinded_block trusts most header fields beyond the three STF-checked fields and withdrawals_root, per its own docs at crates/blockchain/state_transition/src/beacon/block_production.rs:677. That may be intentional at the call boundary, but it makes this function easy to misuse later. If feasible, I’d either:
    • rename it to make the prevalidated-header contract obvious, or
    • add a stronger type / wrapper for a validated builder bid, so callers cannot accidentally pass an unverified header here.
      This is more maintainability/API-hardening than a bug, but for blockchain code it is worth being defensive.

No blocking issues from me based on the diff shown.


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

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review of PR 694: builder-specs containers and assemble_blinded_block

I read the diff but did not build it or run the tests. I did not open the ExecutionPayloadHeader changes in deneb.rs or the vendored fixtures.

The PR is clean. Nothing calls the new public API yet, which limits the risk. I found no blocking bugs.

What checks out

  • Refactor: The assemble_block split into full_body, unsigned_block and post_state keeps the original order. The state still gets apply_pending_mutations() before hash_tree_root().
  • Stand-in payload: assemble_blinded_block copies parent_hash, prev_randao and timestamp from the builder's header, not from the state. That is the right call: copying from the state would let any header through. The code comment explains this.
  • Withdrawals: The explicit withdrawals_root check closes the gap left by the stand-in carrying the expected withdrawals. The test a_header_with_the_wrong_withdrawals_root_is_refused covers it.
  • Patches: latest_execution_payload_header and latest_block_header.body_root are patched after post_state, and blinded.state_root is set after apply_pending_mutations().
  • Fork gate: assemble_blinded_block refuses non-fulu states up front, with a test.
  • Tests:
    • blinded_and_full_blocks_reach_the_same_state_root is a strong guard. It uses distinctive payload fields, a withdrawal due and a deposit request, so a missed patch changes the root.
    • Each of the other checks has its own refusal test.
    • The containers are tested against builder-specs' own example files in both SSZ and JSON.
  • Cleanup: Dropping #[allow(dead_code)] on fulu_state and fixing its docs is good. The function now has callers.

Minor points

  1. Header fields trusted to the caller. assemble_blinded_block only checks withdrawals root, parent hash, prev_randao, timestamp and blob count. The doc and PR description say the caller must still check the bid signature, value, gas limit, blob_gas_used and fork version. Since a later PR carries those checks, I suggest a follow-up test or assertion there, because signing a blinded block over an unchecked header is the main risk in this flow. This is a tracking note, not a defect in this PR.
  2. Duplicate constant values. DOMAIN_APPLICATION_BUILDER and DOMAIN_APPLICATION_MASK are both [0,0,0,1]. That matches the spec, but I couldn't see the end of the diff, so I don't know whether constants.rs tests assert the relationship. If they don't, a one-line assertion would catch the two drifting apart.
  3. Fork coverage. BlindedBeaconBlock::from_parts takes electra::BeaconBlock, while assemble_blinded_block accepts fulu only. This is fine today. The "electra and fulu shapes only" comment should be revisited if an electra-state path is ever added.

Overall this looks good to merge, subject to the base-branch retarget the PR already notes.


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

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR for blinded block production in ethlambda. Let me analyze the key areas carefully.

Overall Assessment

This is a well-structured PR implementing MEV-Boost blinded block production for Fulu. The core approach of using a "stand-in" payload with patched post-state fields is sound and well-documented. However, I've identified several issues ranging from minor to potentially serious.


Critical Issues

1. Stand-in payload uses BeaconBlockBody::empty().execution_payload — potential default value hazard

File: crates/blockchain/state_transition/src/beacon/block_production.rs, line ~724

let stand_in = ExecutionPayload {
    parent_hash: header.parent_hash,
    prev_randao: header.prev_randao,
    timestamp: header.timestamp,
    withdrawals: expected_withdrawals,
    ..BeaconBlockBody::empty().execution_payload
};

Problem: The ..BeaconBlockBody::empty().execution_payload spread fills all unchecked fields with defaults. If ExecutionPayload gains new fields in future forks, or if any field currently not checked by process_execution_payload but written to state is non-default, this creates a mismatch.

The comment table claims only parent_hash, prev_randao, timestamp, and cached header matter, but process_execution_payload also reads fee_recipient, state_root, receipts_root, logs_bloom, block_number, gas_limit, gas_used, extra_data, base_fee_per_gas, block_hash, blob_gas_used, excess_blob_gas for the cached header. The stand-in sets these to defaults (zeros/empty), then patches the cached header afterward. This is correct for state root computation only if those fields don't affect other state writes.

Verification needed: Confirm that process_execution_payload in Fulu doesn't use any of these defaulted fields for state writes beyond the cached header. The latest_execution_payload_header patch replaces the entire header, so this appears safe, but this is a fragile invariant.


2. ExecutionPayloadHeader::from implementation may not match spec's hash_tree_root equivalence

File: crates/common/types/src/beacon/containers/deneb.rs, lines 180-206

impl From<&ExecutionPayload> for ExecutionPayloadHeader {
    fn from(payload: &ExecutionPayload) -> Self {
        Self {
            // ...
            transactions_root: payload.transactions.hash_tree_root(),
            withdrawals_root: payload.withdrawals.hash_tree_root(),
            // ...
        }
    }
}

Problem: The spec's ExecutionPayloadHeader from ExecutionPayload uses SSZ list hashing with proper limits. Need to verify hash_tree_root() on SszList<Transaction, ...> and SszList<Withdrawal, ...> produces the same root as the spec's hash_tree_root(payload.transactions) where transactions is the list type.

Check: The SszList::hash_tree_root should be correct if implemented per spec, but this is consensus-critical. The test at line 536-548 only checks round-trip for a sample payload, not against spec test vectors.


Security Issues

3. assemble_blinded_block trusts execution_payload_header.block_hash without verification

File: crates/blockchain/state_transition/src/beacon/block_production.rs, line ~687-724

The doc comment explicitly states: "the header is trusted as given: the caller verifies everything else it needs from the bid." However, process_execution_payload checks payload.block_hash == hash_tree_root(execution_payload) (in the spec; here the engine verifies). With a stand-in payload, this check is bypassed because the engine receives a constant "accept" rather than the actual payload.

Risk: If the caller doesn't verify block_hash against the header's other fields, a malicious builder could provide an inconsistent header. The doc says caller checks "at least" fork version, signature, value, gas limit, and blob_gas_used — but block_hash binds all header fields together. Missing this check means a header with block_hash != hash_tree_root(reconstructed_payload) could be signed, producing an invalid block that fails gossip validation or fork choice.

Suggestion: Add explicit documentation that block_hash must be verified, or better, compute and verify it from the header fields in assemble_blinded_block before signing.


4. withdrawals_root check is necessary but may be insufficient

File: crates/blockchain/state_transition/src/beacon/block_production.rs, lines 704-710

verify(
    header.withdrawals_root == expected_withdrawals.hash_tree_root(),
    "header.withdrawals_root == hash_tree_root(get_expected_withdrawals(state))",
)?;

This is correct and necessary. However, note that get_expected_withdrawals can fail with Error::SpecAssert for other reasons (e.g., state not Fulu, though that's gated earlier). The error mapping with ? after try_into() could mask the real failure.


Correctness Issues

5. fulu_state error path in assemble_blinded_block could panic or return wrong error

File: crates/blockchain/state_transition/src/beacon/block_production.rs, line ~742

let fields = fulu_state(&mut post, "assemble_blinded_block")?;

If post is not BeaconState::Fulu, this returns Error::SpecAssert("assemble_blinded_block needs a fulu state"). But this is unreachable due to the matches! check at line 692. OK as-is, but consider unreachable! or expect for clarity since it's truly unreachable.


6. latest_block_header.body_root patch uses blinded.body.hash_tree_root() not block.body.hash_tree_root()

File: crates/blockchain/state_transition/src/beacon/block_production.rs, line ~744

fields.latest_block_header.body_root = blinded.body.hash_tree_root();

Wait — blinded.body is BlindedBeaconBlockBody, and block.body was BeaconBlockBody. The BlindedBeaconBlockBody contains execution_payload_header instead of execution_payload.

Critical check: Does BlindedBeaconBlockBody::hash_tree_root() equal BeaconBlockBody::hash_tree_root() when the header is derived from the payload?

The test blinded_and_full_blocks_reach_the_same_state_root verifies this end-to-end, and a_blinded_block_has_the_full_blocks_root checks body roots match. This relies on ExecutionPayloadHeader and ExecutionPayload having the same SSZ root, which is tested. Appears correct.


Performance Issues

7. Unnecessary clone of execution_payload_header in state patch

File: crates/blockchain/state_transition/src/beacon/block_production.rs, line ~743

fields.latest_execution_payload_header = blinded.body.execution_payload_header.clone();

The blinded block is constructed from header which was moved in. After this, blinded owns the header. The clone is needed because blinded is used later. However, you could restructure to avoid this:

let header = inputs.execution_payload_header; // moved
// ... use header directly for stand_in ...
let mut blinded = BlindedBeaconBlock::from_parts(block, header); // header moved in
// Then use std::mem::replace or restructure to avoid clone

Minor issue; the clone is cheap enough for a block production path.


Rust Best Practices

8. verify helper could use ensure! macro pattern

File: crates/blockchain/state_transition/src/beacon/block_production.rs, various lines

The verify function (not shown in diff but used) returns Result<(), Error>. The pattern:

verify(condition, "literal")?;

is fine, but consider if ensure! from anyhow or a custom macro would improve readability. Not a blocker.


9. refusal helper in tests panics on unexpected errors — brittle

File: crates/blockchain/state_transition/src/beacon/block_production.rs, lines 1525-1535

fn refusal(state: &BeaconState, inputs: BlindedBlockInputs) -> &'static str {
    match assemble_blinded_block(...) {
        Err(Error::SpecAssert(what)) => what,
        other => panic!("expected a SpecAssert refusal, got {other:?}"),
    }
}

If assemble_blinded_block returns a different error variant (e.g., SSZ decode error from malformed state, or arithmetic overflow), this panics. Tests should be more granular. However, given the controlled test inputs, this is acceptable.


Documentation and Maintainability

10. Excellent documentation — preserve this pattern

The module-level and function-level docs, especially the step-by-step table in assemble_blinded_block, are outstanding. This is exactly the level of detail needed for consensus-critical code. The blinded_and_full_blocks_reach_the_same_state_root test as a "guard" is well-explained.


Minor Issues

11. distinctive_payload_for test helper uses magic numbers without explanation

File: crates/blockchain/state_transition/src/beacon/block_production.rs, lines 1378-1400

The 0xde, 0xad and 0x11, 0x22, etc. are fine for distinctiveness, but a comment explaining the pattern (each field differs from default/empty) would help.

12. full_inputs includes a DepositRequest in execution_requests — why?

File: `crates/blockchain/state_transition/src/beacon/block_production.rs**, lines 1422-1440

The deposit request in execution_requests is unusual for block production tests unless specifically testing deposit processing. If this is to ensure execution_requests are non-empty and thus contribute to state root, that's fine, but document why.


SSZ/Serialization Correctness

13. ExecutionPayloadHeader now derives serde::Deserialize — check field ordering

File: crates/common/types/src/beacon/containers/deneb.rs, lines 138-149

Adding serde::Deserialize to ExecutionPayloadHeader is needed for the builder API. Ensure the field order in the struct matches the SSZ serialization order (which it should, as SszEncode/SszDecode derive order follows declaration order). The JSON test at json_tests.rs line 376 verifies round-trip.


Summary

Priority Issue Location
High Verify block_hash binding or document caller obligation block_production.rs:687-724
Medium Confirm ExecutionPayloadHeader::from matches spec root exactly deneb.rs:180-206
Medium Stand-in payload default values are fragile to future changes block_production.rs:724
Low Unnecessary clone of header block_production.rs:743
Low Test refusal helper panics broadly block_production.rs:1525-1535

The PR is well-architected and the core approach is sound. The main concern is ensuring all header fields that could affect validity are either checked or documented as caller-checked before signing.


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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants