[MegaRedHand]

🗒️ Description / Motivation

A fulu block whose custody data columns have not all arrived is held until gossip (or a per-slot peer ask) delivers them. The blob transactions such a block commits to usually passed through the local execution client's pool first, so the node can often build those columns itself, sooner. Fulu's p2p-interface.md ("Distributed blob publishing using blobs retrieved from local execution-layer client") asks honest nodes to query engine_getBlobsV2 for exactly this, and Lighthouse and the other clients already do. This adds it to ethlambda beacon.

hold_block_for_columns (first hold of a fulu block with blob commitments, engine configured)
  └─ spawn task: engine_getBlobsV2(versioned hashes)        one attempt, 1 s
       null / error ─► count, give up (gossip and peer asks carry on)
       every blob ──► spawn_blocking:
                        imported meanwhile? / nothing missing? ─► stop
                        build the missing custody columns (no KZG verification)
                      check_data_column_sidecars(sidecars, SidecarOrigin::ExecutionLayer)
p2p (ExecutionLayer origin):
  record in seen_columns ─► publish on subnet (custody = subscribed) ─► new_data_column_sidecars
chain actor: store ─► release_block_if_columns_complete

What Changed

Piece	Where
engine_getBlobsV2, BlobAndProofV2, wire-shape checks, capability list	crates/net/engine
unverified_sidecars: builds only the requested columns, sharing code with verified_sidecars	crates/blockchain/state_transition/src/beacon/data_columns.rs
Trigger and fetch task	crates/blockchain/src/beacon_blobs.rs, hook in hold_block_for_columns
SidecarOrigin { Parked, ExecutionLayer } on the existing check_data_column_sidecars	crates/net/api
Seen cache, publish, forward for ExecutionLayer sidecars	crates/net/p2p/src/beacon/engine_columns.rs
Docs and metrics	docs/beacon_engine.md (new section), docs/metrics.md, CLAUDE.md

Design decisions:

• No new BlockChainToP2P method. The existing check_data_column_sidecars gains a SidecarOrigin. Parked keeps today's behavior (chain checks); ExecutionLayer skips the checks and does what the spec requires instead. The seen cache and publishing live in p2p, so that is where they happen.
• One attempt, 1 s (osaka.md's timeout), outside the engine retry ladder: the call races gossip delivery of the same columns, so a retry after the ladder's backoff would land after that race is decided.
• V2, not V3. V3 answers partially, but a column needs one cell from every blob; without partial-column assembly a partial blob set builds no complete column.
• No KZG verification of the execution client's proofs, as the spec recommends ("SHOULD skip verification") and Lighthouse does (from_execution_verified).
• Publish iff subscribed: only custody columns are built and published, since this node's column subnets are its custody columns.
• No CLI flag: on whenever --execution-endpoint is set.

Not covered (follow-ups): asking on a data_column_sidecar's arrival (the spec's second trigger); gloas, where availability moves to the payload envelope; partial columns (V3).

Open questions for review:

• No age gate or concurrency cap. Every first hold asks, whatever the block's age or source, so range catch-up issues one getBlobsV2 per held block (up to a batch's worth concurrently, each capped at 1 s). A syncing execution client answers null at once, so this is cheap in practice, but it is an unbounded fan-out. Alternatives: ask only for blocks near the tip (slot + N >= current_slot), or a small semaphore.
• Supernodes publish in one burst. A node custodying every column publishes all of them at once on a hit. Lighthouse spreads this out (publish_data_columns_gradually) to avoid a bandwidth spike.

New metrics: lean_engine_get_blobs_total{outcome}, lean_engine_get_blobs_duration_seconds, lean_engine_blobs_to_columns_seconds, lean_data_columns_from_execution_layer_total{result}.

Correctness / Behavior Guarantees

• Nothing changes without --execution-endpoint: the hook needs an engine client.
• Parked sidecars take exactly the old path.
• An ExecutionLayer sidecar is recorded in seen_columns before it is published, so a later gossip copy of the same (slot, proposer, index) is ignored, and a column gossip already accepted is neither republished nor re-forwarded.
• The fetch runs off the chain actor (async task plus spawn_blocking for the cells) and re-checks, just before building, that the block is still unimported and which custody columns are still missing.
• Trust trade-off: an execution client that returns wrong proofs makes this node store and publish columns peers reject. Debug builds re-run the chain checks on every stored sidecar, so tests would catch a malformed build.

Tests Added / Run

• ethlambda-engine: wire round trips for a null answer (and the request's method and params), a full answer in order, a wrong proof count, a length mismatch; unit tests for a wrong blob size and a null element.
• data_columns: requested columns equal the matching entries of verified_sidecars, ascending and deduplicated; empty inputs; shape errors; a tampered proof still builds (nothing is verified).
• engine_columns: newly seen custody columns are recorded and forwarded in one batch; an already seen column is skipped; a non-custody column is dropped; nothing new sends nothing.
• beacon_blobs: against a mock execution client serving real KZG blobs and proofs: a full answer hands over exactly the custody columns; a stored column is not handed over again; a null answer hands over nothing; no engine asks nothing; a second hold does not ask again.

cargo test -p ethlambda-engine --profile release-fast
cargo test -p ethlambda-state-transition --lib --profile release-fast beacon::data_columns
cargo test -p ethlambda-p2p --lib --profile release-fast beacon::engine_columns
cargo test -p ethlambda-blockchain --lib --profile release-fast beacon_blobs
cargo test -p ethlambda-blockchain --lib --profile release-fast hold
make lint

Not yet run against a live execution client.

Related Issues / PRs

• None

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — not run; the per-crate tests above were run instead

[github-actions]

🤖 Claude Code Review

Review of PR 769: execution-client blob fetching (engine_getBlobsV2)

The structure is clean. Splitting out column_sidecar, check_shape and compute_all_cells lets the verified and unverified builders share one assembly path, and the tests are thorough. The main concerns are about when the fetch fires, because the hit path publishes columns and fills the anti-equivocation cache without running any checks.

Correctness / security

1. The fetch fires for blocks whose proposer signature may not have been checked yet (lib.rs, hold_block_for_columns hunk). The spec trigger is a valid beacon_block or data_column_sidecar from gossip. Gossip blocks have passed proposer-signature validation by the time they are held. Sync (req/resp) blocks have not; the signature is only checked at state transition. On a hit, engine_columns::publish_and_forward:

   • records (slot, proposer_index, index) in seen_columns;
   • publishes sidecars whose signed_block_header.signature is the unverified block signature.

   So a peer serving a forged or invalid block during sync can trigger two problems:

   • Publishing invalid columns. Peers will REJECT them, which costs this node peer score.
   • Seen-cache poisoning. The real block's valid columns for the same (slot, proposer, index) are then IGNOREd on gossip.

   Suggest gating on the block having passed gossip validation, or verifying the proposer signature first. The chain actor already has the parent state when it holds for DA, so that check is cheap.

2. Fetch concurrency is unbounded, and the fetch is not limited to recent slots (beacon_blobs.rs, spawn_fetch; docs/beacon_engine.md says "Any slot and any source qualifies").

   • During range sync, each held fulu block spawns its own getBlobsV2 call against the EL.
   • Each hit spawns a spawn_blocking job that does rayon par_iter cell computation, so they compete for CPU.
   • For old blocks the EL pool will almost always miss, so this is mostly wasted EL load.
   • Columns that do get built for old slots are mostly IGNOREd by peers anyway.

   Suggest two limits:

   • only ask when slot is within a small window of the wall-clock slot;
   • cap in-flight fetches with a tokio::sync::Semaphore.

3. Race with in-flight gossip validation (engine_columns.rs, seen_columns.record). If gossip received a copy whose verdict is still pending (queued or on the spawn_blocking checker) and does not record into seen_columns until it is accepted:

   • our publish succeeds, or hits gossipsub's duplicate cache;
   • the peer's copy is then IGNOREd, which denies that peer P3 credit.

   This is minor. Please confirm whether seen_columns is recorded on accept or on receipt, and document the behaviour.

4. The "Built data columns" info! is logged even when the hand-over failed (beacon_blobs.rs, end of fetch_and_publish). Return early on Err from check_data_column_sidecars, or move the log above the call.

Rust / maintainability

5. Inconsistent path for KzgProof (types.rs). KzgProof is imported at the top, but BlobAndProofV2::proofs spells out the full path ethlambda_types::beacon::primitives::KzgProof. Use the import.

6. types.rs now depends on building.rs for parse_data and parse_fixed, which only became pub(crate) in this PR. These are generic hex/DATA helpers. Consider moving them to a small hex/wire module, or into types.rs next to data(), so the type definitions do not depend on the payload-building module.

7. blob_count = entries.len() is captured before entries is moved. That is fine. The blobs.len() != commitments.len() check in check_shape is redundant with decode_blobs_v2's length check, which is harmless defence in depth.

8. Possibly quadratic present.contains(index) (beacon_blobs.rs). If data_column_indices_for returns a Vec, the lookup is O(n·m). That is negligible at 128 columns, but a HashSet would make the intent clearer.

Tests

9. Several beacon_blobs tests rely on fixed sleeps (200–300 ms) to assert that something did not happen:

   • a_null_answer_hands_nothing_over
   • without_an_engine_nothing_is_asked_or_handed_over
   • a_second_hold_of_the_same_root_does_not_ask_again

   These are acceptable, but can be flaky on loaded CI. Waiting on the request counter first, as the null test already partly does, and then yielding is more robust.

10. Missing test: a hit where has_state(block_root) is already true, i.e. the block was imported while the EL answered. It should hand nothing over; this path is untested.

Docs

The docs are accurate and the "Trust" section is a good addition. Once Item 1 or Item 2 is addressed, update the "Any slot and any source" sentence in beacon_engine.md to match.

---

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

[github-actions]

🤖 Codex Code Review

• [P1] Validate the block header before publishing columns or updating the seen cache — engine_columns.rs:63–66. Sync blocks reach hold_block_for_columns before signature validation or state transition. A malicious peer can supply a forged block referencing blobs already in the local execution client; this path then publishes sidecars carrying its invalid signature and records (slot, proposer, column) as seen. This can suppress legitimate columns and damage this node’s peer score. Verify the header signature and expected proposer before recording or publishing; trusting local KZG proofs does not validate a peer-supplied header.

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

---

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

[github-actions]

🤖 Grok Code Review

The execution-layer blob path is well structured, and the V2-only, one-shot, 1s timeout matches the spec. A few consensus and liveness issues should be fixed before this is relied on.

1. Wrong blobs still complete data availability. unverified_sidecars (data_columns.rs ~275) only checks counts and column indices. fetch_and_publish never checks that each blob’s KZG commitment (or versioned hash) matches blob_kzg_commitments. SidecarOrigin::ExecutionLayer then skips every column check and the chain stores what it gets. A buggy or compromised EL can make this node treat DA as satisfied, import the block, and attest to data peers will reject. Peer-score loss, as docs/beacon_engine.md describes, is the mild outcome. Compare commitments in the blocking task and, on mismatch, do not hand the sidecars to p2p so gossip and peer asks stay open. Spec “SHOULD skip verification” is not a reason to skip the binding between the block and the blobs.

2. Seen-cache update is not tied to a successful store. In engine_columns.rs (~64–72) seen_columns.record runs before publish and before new_data_column_sidecars (~118–131). If the chain ref is None, the send fails, or the actor drops the batch, that (slot, proposer, index) is poisoned: a later gossip copy is ignored, and this node may never store the column. Record only after the chain accepts the batch, or roll the seen key back on send failure. Log the blockchain == None path; it currently drops the batch silently.

3. Every historical hold hits the EL. hold_block_for_columns (lib.rs ~3180) spawns an unbounded task for any fulu block with commitments, including sync, and each task clones the full block across a 1s RPC. The blob pool will miss almost all of those, but the calls still occupy EngineClient. If call_once shares a connection or inflight limit with newPayload / forkchoiceUpdated, backfill can stall fork choice for a second per held block, and memory tracks every in-flight payload. Gate on a recent slot (blob-pool retention / gossip window) and cap inflight fetches. The new comment that this call “races nobody” is also wrong; the module docs correctly say it races gossip.

4. No test covers the bytes actually published. publish_and_forward publishes sidecar.to_ssz() on the p2p actor (engine_columns.rs ~86). That must be the inner fulu gossip SSZ (then compress_message), not an enum encoding, and only when that subnet topic is actually subscribed. Custody is only a proxy for subscription and can lag a custody update. A unit test that decodes the published payload the same way gossip does would lock this down.

Smaller notes: lean_engine_get_blobs_total{hit} is incremented before the blobs are checked against the block, so shape failures look like hits. Debug-only chain_checks after new_data_column_sidecars will not roll back a block already released from the hold; do not treat that as a safety net.

---

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

[github-actions]

🤖 Gemini Code Review

The implementation of engine_getBlobsV2, unverified_sidecars, and the P2P hand-over is well-structured and cleanly tested. However, there is one important security/correctness issue regarding unvalidated blocks from Req/Resp sync reaching hold_block_for_columns.

Issues

1. Unverified blocks from Req/Resp can trigger engine_getBlobsV2, poison seen_columns, and publish invalidly-signed sidecars to gossip (crates/blockchain/src/lib.rs:3182-3195, crates/blockchain/src/beacon_blobs.rs:134-184, crates/net/p2p/src/beacon/engine_columns.rs:53-94)

   hold_block_for_columns runs in process_or_pend_block before state_transition verifies the block's proposer index and signature. While blocks arriving via gossip have already had their proposer signature validated in p2p, blocks arriving via Req/Resp (BeaconBlocksByRange / BeaconBlocksByRoot) have not.

   Because spawn_fetch runs for any held block regardless of source and SidecarOrigin::ExecutionLayer skips all header checks in engine_columns::publish_and_forward:

   • A peer answering a Req/Resp request can send a block with a known parent_root, the current slot and honest proposer_index, an invalid signature, and blob_kzg_commitments taken from public blob transactions currently in the execution mempool.
   • engine_getBlobsV2 will succeed (since the blobs are in the local EL's pool), and unverified_sidecars will wrap them in the forged block's header.
   • engine_columns::publish_and_forward will then:
     1. Record (header.slot, header.proposer_index, index) in server.seen_columns, which is keyed by (slot, proposer_index, index) rather than block_root — causing the honest proposer's real columns on gossip to be dropped as already_seen.
     2. Publish the sidecars carrying the invalid block header signature to data_column_sidecar_{subnet_id}, causing peers on the mesh to reject them and penalize/ban this node.
     3. Forward the sidecars to new_data_column_sidecars, which in debug builds re-runs column::chain_checks and will panic on the invalid header signature.

   Suggestion: Verify the block header (slot > parent slot, expected proposer_index, and proposer BLS signature against the parent state, or only trigger spawn_fetch for gossip-validated blocks) before building/publishing sidecars as SidecarOrigin::ExecutionLayer.

Minor

2. Pre-allocate proofs vector (crates/blockchain/src/beacon_blobs.rs:162)

   let mut proofs: Vec<KzgProof> = Vec::with_capacity(entries.len() * CELLS_PER_EXT_BLOB);

   Each entry has CELLS_PER_EXT_BLOB (128) proofs, so pre-allocating avoids repeated reallocations in proofs.extend(entry.proofs).

---

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

[github-actions]

🤖 DeepSeek Code Review

Overall this is a clean refactor (column_sidecar extraction, the SidecarOrigin split, and the wire decoding are all readable). The main concerns are around the trusted execution-layer path and resource bounds.

Findings:

• crates/net/p2p/src/beacon/engine_columns.rs in the publish loop: server.swarm_handle.publish(...) ignores its Result, but published is incremented unconditionally. If publish returns Err (for example insufficient peers), the column is still marked seen and forwarded, but the metric/log claims it was published. Consider inspecting the result and counting publish failures separately.

• crates/blockchain/src/beacon_blobs.rs in fetch_and_publish: spawn_fetch launches an unbounded task per first hold, and every engine hit then launches a spawn_blocking cell-computation task. During sync or a burst of held Fulu blocks this can create many concurrent 1 s engine calls plus CPU-heavy cell jobs. A semaphore or bounded worker/queue would be safer.

• crates/blockchain/state_transition/src/beacon/data_columns.rs in unverified_sidecars: the execution-layer path skips KZG proof verification and also does not verify that the returned blobs correspond to block.message.body.blob_kzg_commitments / the requested versioned hashes. That is documented and spec-allowed for a trusted local EL, but a buggy or malicious EL can make the node store and gossip invalid sidecars and lose peer score before anything catches it. A cheap versioned-hash/commitment sanity check would reduce the blast radius without full proof verification.

• crates/net/engine/src/client.rs in get_blobs_v2: the call is not gated by engine_exchangeCapabilities. If the EL does not advertise engine_getBlobsV2 or the network is not Osaka yet, every held Fulu block will produce an error/miss metric and a wasted attempt. Consider using negotiated capabilities to disable the path or warn once.

• crates/net/p2p/src/beacon/engine_columns.rs in publish_and_forward: sidecars are recorded in seen_columns before the chain handover. If server.blockchain is None or new_data_column_sidecars fails, the column is already marked seen and will never be retried through this path. That is probably intentional for duplicate suppression, but the failure mode should be explicit/metric’d.

Minor:

• crates/net/engine/src/types.rs now imports from building.rs, which likely already imports from types.rs; this intra-crate cycle works but is a bit tangled. Moving parse_data/parse_fixed to a small shared module would be cleaner.
• In beacon_blobs.rs, proofs could be pre-allocated with Vec::with_capacity(entries.len() * preset::CELLS_PER_EXT_BLOB) rather than growing from zero.

---

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

[github-actions]

🤖 Kimi Code Review

Findings

1. Unbounded fetch and reconstruction fan-out — crates/blockchain/src/lib.rs:3180-3195, crates/blockchain/src/beacon_blobs.rs:76-87,136
   Every distinct held root spawns an independent engine request, and every successful response can queue a spawn_blocking job that computes cells for all blobs. During sync, or when peers systematically withhold columns, many valid blocks can be held quickly, producing numerous concurrent engine requests and a large backlog of blocking tasks and blob-filled responses. The import re-check limits wasted computation once a job starts, but does not bound queued work or memory. Consider a bounded worker or semaphore acquired before spawning, with work dropped or coalesced when saturated.

2. Potential test race when waiting for the hand-over — crates/blockchain/src/beacon_blobs.rs, wait_for_hand_over; crates/blockchain/src/lib.rs:4727-4731
RecordingP2P pushes to checks, releases that mutex, and only then pushes to check_origins. wait_for_hand_over waits solely on checks, so a_full_answer_hands_over_exactly_the_custody_columns can theoretically observe the sidecars before the origin is recorded and fail intermittently. Record the origin before releasing the checks update, use one mutex for both vectors, or wait for both.

Otherwise, the implementation looks careful: strict wire-shape validation, correct V2 all-or-nothing handling, bounded/deduplicated column construction, and seen-cache recording before publication all align well with the intended Fulu flow.

---

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