[MegaRedHand]

🗒️ Description / Motivation

A range or by-root answer, or a batch of parked columns replayed after their parent imports, carries many columns of the same block. Each column was checked on its own blocking task with its own verify_cell_kzg_proof_batch call, and the chain actor heard nothing until every sidecar of the whole call was done, so data availability waited on the slowest block of the batch. And every fulu sidecar of a block carries the same signed header, so its BLS signature was verified once per sidecar.

What Changed

Two commits:

1. One KZG batch per block (p2p/src/beacon/column_checks.rs, state_transition/src/beacon/gossip/column.rs, and fork_choice::verify_column_cells_batch, one verify_cell_kzg_proof_batch call over every cell of several columns): check_and_forward groups the sidecars by block (the signed header's root for fulu, beacon_block_root for gloas). A group takes one column_check_permits permit, runs every sidecar's rules that do not read the cells, then verifies the proofs of all sidecars still standing in one batch. The group's Keep and AwaitParent sidecars go to the chain actor as soon as the group finishes. chain_checks is now chain_precheck followed by a KZG step, and gossip's stateful_checks/stateful_checks_gloas keep their order through the same split halves.
2. Shared header signature verified once per group: SignatureMemo remembers header_signature_is_valid's answer for the group, keyed on the header root, the signature bytes and (for the head-state check made before parking) the head it read, so a forged signature over a shared header is judged on its own. The memo lives in one group's spawn_blocking closure; gossip and the single-sidecar wrappers use a fresh one per call.

Correctness / Behavior Guarantees

• Verdicts do not change. The proofs are the last rule on the chain path (the lookahead rule after them is gossip's alone). A batch that fails is re-run sidecar by sidecar, so exactly the sidecars rejected before are rejected.
• Only the header signature's answer is memoized; every other rule runs per sidecar in its order.

Tests Added / Run

• Measured locally: a batch over 9-blob columns costs 3.3x less than separate calls at 8 columns and 4.8x less at 32.
• cargo fmt --all -- --check: clean
• make lint: clean
• cargo test --profile release-fast -p ethlambda-state-transition -p ethlambda-p2p --lib --bins: state-transition 634 passed (16 ignored), p2p 364 passed (1 ignored)
• Not run locally: make test (spec suites); CI covers them.

Related Issues / PRs

Part of a beacon import perf batch, each PR independent and based on main: #752 (validation contention metrics, also edits column_checks.rs and docs/metrics.md), #753 (merkle layer hashing). The engine/STF overlap PR also edits fork_choice.rs, in a different region.

✅ 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 locally (spec suites); unit tests of the touched crates pass

[github-actions]

🤖 Claude Code Review

Review: PR 754, one KZG batch per block for fetched data columns

I read the diff only. I did not build it or run the tests. Overall this looks good, and I found nothing blocking.

Correctness

• Verdict equivalence holds. The proofs are the last rule on the chain path. The lookahead rule that follows them is PastLookahead::Queue's alone, and it stays after the KZG step in judge_against_parent (column.rs, around line 485). So chain_precheck followed by kzg_verdicts gives each sidecar the verdict it had before.
• The fallback is sound. If the batch fails, kzg_verdicts re-verifies each sidecar alone. A malformed entry (Cell::from_bytes error or length mismatch) turns into Err, which also takes the fallback path. Neither a forged column nor a malformed one can get a neighbour rejected or accepted wrongly.
• Batch inputs are length-checked beforehand. verify_column_cells_batch relies on equal lengths for cells, commitments and proofs. verify_data_column_sidecar (fulu) and gloas_verify_data_column_sidecar (gloas, in gloas_before_kzg) both run before a sidecar can become Pending. A violation would only fail the batch and trigger the fallback, not corrupt a verdict.
• The SignatureMemo key is sound. The key is (header_root, signature, head). A forged signature over a shared header gets its own entry. The head-state check in queue_unless_forged is keyed on the head it read, so the same header judged against a different state is a separate entry. The test a_groups_shared_header_signature_is_verified_once covers the forged case. A memo entry can also be shared across sidecars whose parent state appeared midway, because the parent-state and head-state paths use different keys (None versus Some(head)).
• Grouping is safe. Fulu sidecars are grouped by header root, and the inclusion proof ties their commitments to that root. Gloas sidecars are grouped by beacon_block_root and take their commitments from the bid. So the commitments within a group are consistent.
• Order is preserved. Only Pending sidecars can become Keep, and they are pushed in input order. Keep and AwaitParent order within a group therefore matches arrival order.

Things to consider (non-blocking)

1. Panic handling is split.
   • In judge_group, a panic in the precheck is caught by catch_unwind per sidecar, so it costs only that sidecar.
   • A panic in kzg_verdicts, or in the batch itself, loses the whole group via the outer spawn_blocking join error (column_checks.rs).
   • Both are reasonable and documented. The catch_unwind relies on panic=unwind, which the old spawn_blocking path also relied on, so nothing changes there.
   • A short comment on the second case in judge_group would help. The existing comment says "costs that one sidecar", which is not true of the KZG step.
2. A bad column in a block costs more than before. One bad column that passes the precheck makes the group pay one batch plus N single verifications. By the PR's own 3.3–4.8x measurement that is about 1.2–1.3x the old per-sidecar cost, which is acceptable. It might be worth noting that the peer sending the bad column is not penalized here. That is the same as before, so I am not asking for a change.
3. Metric semantics changed. lean_data_column_kzg_verify_seconds now observes per batch. On failure it also gets one observation per re-verified sidecar. docs/metrics.md documents this, but dashboards or alerts built on per-sidecar latency will shift.
4. SignatureMemo.verified is a test-only counter in a production struct. It is a cheap usize, so this is fine. If you want to keep it out of the hot struct, #[cfg(test)] would do it.
5. Memory. The batch copies every cell (c_kzg::Cell::from_bytes, about 2 KiB each) into one Vec. This is bounded by columns times blobs per block and similar to before, but the copies now live at once per group. With several groups in flight (bounded by column_check_permits) that is still modest.

Tests and docs

• The new tests cover the valid batch, one bad column in a batch, swapped proofs, grouping order, per-block forwarding, and signature-memo reuse. These are the right cases.
• docs/beacon_wire.md and docs/metrics.md are updated to match.

Verdict: approve. Item 1 is the only point I would act on, and it is just a comment tweak.

---

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

[github-actions]

🤖 Codex Code Review

I found one substantive issue; otherwise the batching approach looks sound and preserves per-sidecar verdicts.

• Potential DoS / unbounded task fan-out in crates/net/p2p/src/beacon/column_checks.rs:163. check_and_forward spawns one outer task, then spawns one inner Tokio task per block-group before those tasks enter spawn_blocking. The semaphore limits blocking work, but not the number of queued async tasks or retained groups. A large DataColumnsByRange response with many distinct block roots can therefore allocate one owned task + one moved Vec<DataColumnSidecar> per group and keep them pending indefinitely. For p2p-facing blockchain code, that is a meaningful memory-pressure vector. Consider processing groups in the outer task directly after acquiring each permit, or adding a bounded work queue so permits bound both execution and queued work.

• Batch verifier relies on a non-local invariant without enforcing it in crates/blockchain/state_transition/src/beacon/fork_choice.rs:4152. verify_column_cells_batch assumes column.len() == commitments.len() == proofs.len() for every entry, but it only precomputes total from column.len() and then independently appends commitments/proofs. Today callers appear to pre-validate shape, so this is probably correct, but the function is pub and a future caller can silently build mismatched vectors and push malformed input into verify_cell_kzg_proof_batch. I’d recommend either making this helper private to the module that preserves the invariant, or asserting/returning an error when lengths differ.

• Panic handling drops data silently after partial progress in crates/net/p2p/src/beacon/column_checks.rs:104 and crates/net/p2p/src/beacon/column_checks.rs:173. I understand the intent, but catching panics and converting them into dropped sidecars/groups means transient storage bugs or invariant violations can cause permanent data loss rather than backpressure/retry. In consensus-adjacent ingestion, that tradeoff is risky. At minimum I’d want stronger telemetry here (group size / block root / fork) and ideally a clearer distinction between malformed peer input and local internal failure.

• Performance/readability note: group_by_block in crates/net/p2p/src/beacon/column_checks.rs:75 is fine functionally, but it materializes Vec<Vec<_>> for the whole batch before any work starts. Since forwarding happens per group anyway, a streaming approach would reduce peak memory and simplify the task-fan-out concern above.

• What looks good

  • kzg_verdicts in crates/blockchain/state_transition/src/beacon/gossip/column.rs:428 preserves correctness by falling back to per-sidecar verification on batch failure, which is the right safety property.
  • Sharing SignatureMemo across sidecars of one block is a sensible optimization and does not appear to weaken signature validation semantics.
  • Grouping by block root/header root matches the batching assumption for shared KZG/signature work.

• Validation note

  • I attempted a targeted test run, but the environment’s Rust toolchain cache is read-only, so cargo test could not execute here.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces KZG batch verification for data column sidecars grouped by block. Let me analyze the changes systematically.

Overall Assessment

This is a well-structured optimization that batches KZG proof verification across multiple data column sidecars of the same block, reducing cryptographic overhead. The design is sound but I have several concerns ranging from correctness edge cases to API safety.

---

Critical Issues

1. SignatureMemo key uses BlsSignature which may not implement Hash/Eq correctly for caching

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 841-870

let key = (
    header.hash_tree_root(),
    sidecar.signed_block_header.signature,
    head,
);

Problem: BlsSignature is likely a wrapper around byte arrays or cryptographic types. The Hash and Eq implementations must be canonical—bitwise equality of the serialized form. If BlsSignature uses compressed representation with possible non-canonical encodings (e.g., different byte strings that represent the same signature point), memoization could fail to match equivalent signatures or worse, match distinct ones.

Recommendation: Verify BlsSignature's Hash and Eq derive on the compressed serialization, not any internal representation. Add a comment asserting this invariant.

---

2. cell_indices construction in verify_column_cells_batch may overflow usize on 32-bit systems

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 4150-4155

let total: usize = columns.iter().map(|column| column.column.len()).sum();

Problem: On 32-bit systems, usize is u32. With many columns × many cells per column, this could overflow. The subsequent Vec::with_capacity(total) would panic or allocate incorrectly.

Recommendation: Use checked_sum() or explicit overflow check, or document the 64-bit platform assumption. Given this is consensus code, defensive programming is warranted:

let total: usize = columns.iter()
    .map(|column| column.column.len())
    .try_fold(0usize, |acc, len| acc.checked_add(len))
    .ok_or_else(|| Error::SpecAssert("column cell count overflow"))?;

---

3. catch_unwind around chain_precheck_for_with may unwind across FFI boundaries unsafely

File: crates/net/p2p/src/beacon/column_checks.rs, lines 115-122

let precheck = catch_unwind(AssertUnwindSafe(|| {
    column::chain_precheck_for_with(store, &sidecar, now_ms, &mut memo)
}));

Problem: AssertUnwindSafe is used to bypass the UnwindSafe bound, but Store contains internal mutability (likely RwLock, Mutex, or atomic operations). If a panic occurs while memo or store internal state is modified, subsequent use is undefined behavior.

More critically: if c_kzg or BLS verification uses FFI (highly likely), unwinding through FFI is undefined behavior in Rust. The AssertUnwindSafe doesn't fix this—it just suppresses the lint.

Recommendation:

• Ensure c_kzg operations happen outside this catch_unwind (they don't—they're in kzg_verdicts later, which is fine)
• But store reads may trigger DB operations that use FFI (RocksDB, LMDB, etc.)
• Consider moving the catch_unwind to only wrap the pure Rust logic, or document that Store operations must be panic-safe

Actually, re-reading: the catch_unwind is only around chain_precheck_for_with, not kzg_verdicts. The precheck does store reads. If the store's get_state or similar panics, AssertUnwindSafe is claiming this is safe. This is a significant safety claim.

---

Security Vulnerabilities

4. SignatureMemo does not bound memory consumption—potential DoS vector

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 841-870

Problem: A malicious peer could send many sidecars with unique (root, signature, head) combinations, causing unbounded HashMap growth within a single spawn_blocking task. While the memo is dropped per-task, a single task processing many malicious messages could OOM.

Recommendation: Add a size limit with LRU eviction, or document why this is bounded (e.g., number of sidecars per block is protocol-limited).

---

5. KZG batch failure fallback does not preserve timing side-channel resistance

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1374-1399

if matches!(batch, Ok(true)) {
    return vec![ChainVerdict::Keep; items.len()];
}
refs.iter()
    .zip(items)
    .map(|(sidecar, (_, pending))| kzg_verdict_ref(sidecar, pending))
    .collect()

Problem: The fallback to individual verification on batch failure creates a timing oracle. A malicious block with one forged column causes:

1. One fast batch failure (cryptographic rejection)
2. N individual verifications

An observer measuring lean_data_column_kzg_verify_seconds or wall-clock time can distinguish "all valid" from "one forged" based on timing. This leaks information about which columns are invalid.

Recommendation: This is documented behavior and may be acceptable, but consider:

• Constant-time fallback (always verify all individually, defeating the optimization)
• Or document this as a known limitation for non-gossip paths (gossip is single-message)

Actually, re-reading: this is for req/resp, not gossip. The doc says "Gossip keeps one message, one verdict, and is not batched." So this timing channel affects range sync. Acceptable with documentation.

---

Correctness Issues

6. kzg_verdicts short-circuit for single item bypasses batch path but changes metrics behavior

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1374-1376

if let [(sidecar, pending)] = items {
    return vec![kzg_verdict(sidecar, pending)];
}

Problem: For a single item, this calls kzg_verdict which does verify_column_cells_batch(&[...]) with a single-element slice. But the metrics comment says "one observation per sidecar if that batch fails." For the single-item case, there's no batch-then-fallback path, so no double-counting. However, the short-circuit means kzg_verdict uses sidecar.cells() with different lifetime constraints.

Actually, this looks correct. But note that kzg_verdict allocates a new Vec for the single-element slice. The optimization is minor.

---

7. ColumnRef::cells lifetime annotation may be unsound

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1294-1312

fn cells(&self, commitments: &'a [KzgCommitment]) -> fork_choice::ColumnCells<'a> {

Problem: The &'a [KzgCommitment] borrows for 'a, but ColumnRef<'a> already borrows the sidecar for 'a. The returned ColumnCells<'a> captures both. However, in kzg_verdicts:

let refs: Vec<ColumnRef<'_>> = items.iter().map(...).collect();
let columns: Vec<fork_choice::ColumnCells<'_>> = refs
    .iter()
    .zip(items)
    .map(|(sidecar, (_, pending))| sidecar.cells(&pending.commitments))
    .collect();

Here refs has anonymous lifetime tied to items, and columns borrows from refs and items. The borrow checker accepts this, but it's fragile. If refs and columns are reordered or refs is dropped early, this fails.

Recommendation: Add a comment explaining the lifetime dependency chain, or refactor to avoid the two-phase construction.

---

8. group_by_block uses HashMap without deterministic ordering guarantee for tie-breaking

File: crates/net/p2p/src/beacon/column_checks.rs, lines 173-186

fn group_by_block(sidecars: Vec<DataColumnSidecar>) -> Vec<Vec<DataColumnSidecar>> {
    let mut positions: HashMap<Root, usize> = HashMap::new();
    // ...
}

Problem: The test sidecars_are_grouped_by_block_in_first_seen_order verifies first-seen ordering, but HashMap insertion order is preserved in std::collections::HashMap as of Rust 1.36+. However, this is an implementation detail, not a guarantee. If the hasher changes or hashbrown is updated, ordering may change.

Recommendation: Use IndexMap or document reliance on HashMap's insertion ordering. Or use a Vec with linear search for deterministic behavior.

---

Performance Issues

9. kzg_verdicts allocates refs and columns vectors unnecessarily

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1380-1388

let refs: Vec<ColumnRef<'_>> = items.iter().map(|(sidecar, _)| (*sidecar).into()).collect();
let columns: Vec<fork_choice::ColumnCells<'_>> = refs
    .iter()
    .zip(items)
    .map(|(sidecar, (_, pending))| sidecar.cells(&pending.commitments))
    .collect();

Problem: Two intermediate allocations. For the common case of batch success, only columns is needed. For fallback, both refs and columns are needed.

Recommendation: Could construct columns directly with a single pass if ColumnRef is embedded, but the lifetime trickery makes this hard. Acceptable as-is, but consider:

// Single allocation: Vec with capacity for both
let mut columns = Vec::with_capacity(items.len());
// ... build directly

Actually, ColumnRef is needed for fallback. Keep as-is, but consider SmallVec or similar for typical small N.

---

10. verify_column_cells_batch eagerly converts all cells before checking lengths

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 4155-4168

for cell in column.column {
    cells.push(
        c_kzg::Cell::from_bytes(&cell[..])
            .map_err(|_| Error::SpecAssert("len(cell) == BYTES_PER_CELL"))?,
    );
}
commitments.extend_from_slice(column.commitments);
proofs.extend_from_slice(column.proofs);

Problem: If commitments.len() != column.column.len() or proofs.len() != column.column.len(), the error from verify_cell_kzg_proof_batch will be cryptic. The doc comment says "Each column's three lists must have equal length, as verify_data_column_sidecar and its gloas counterpart guarantee"—but this is an internal function with a public API.

Recommendation: Add debug assertions or explicit length checks with clear error messages:

assert_eq!(column.commitments.len(), column.column.len(), "commitments mismatch");
assert_eq!(column.proofs.len(), column.column.len(), "proofs mismatch");

Or return a descriptive Error.

---

Rust Best Practices

11. SignatureMemo::answers uses Result<(), Outcome> as value type—Outcome is already the error case

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 841-870

answers: HashMap<(Root, BlsSignature, Option<Root>), Result<(), Outcome>>,

Problem: Outcome includes Reject and Ignore variants. Result<(), Outcome> is semantically odd—success is Ok(()), failure carries Outcome. But Outcome::Accept exists too, making this a tri-state.

Actually, looking at usage: header_signature_is_valid returns Result<(), Outcome> where Ok(()) means "signature valid, continue checks" and Err(outcome) means "verdict decided." The memo caches this exactly.

This is acceptable but the type alias or a custom enum would be clearer:

enum SignatureCheck {
    Valid,           // Ok(())
    Decided(Outcome), // Err(outcome)
}

---

12. PendingKzg has private field but is pub—intentional?

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1270-1274

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PendingKzg {
    commitments: Vec<KzgCommitment>,
}

Problem: Field is private, struct is public. No constructor is public. But chain_precheck_with returns ChainPrecheck::Pending(PendingKzg { commitments: ... }) within the same module, and kzg_verdicts takes &PendingKzg.

External callers can only use PendingKzg as an opaque type in kzg_verdicts. This seems intentional for API control.

Acceptable, but consider pub(crate) or a constructor if tests need it.

---

13. judge_group drops sidecars on panic without metrics

File: crates/net/p2p/src/beacon/column_checks.rs, lines 115-122

Err(_) => error!("A data column check panicked; dropping the sidecar"),

Problem: The sidecar is silently dropped with no metrics increment. Other drops are counted via count_drop. Panics are exceptional but should be observable.

Recommendation: Add a metric or at least increment a counter for panicked checks.

---

Documentation Issues

14. Doc comment typo: "gloas" vs "gloas counterpart"

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, line 4148

/// [`verify_data_column_sidecar`] and its gloas counterpart guarantee.

Should be "its gloas counterpart" → already says "gloas". Actually "gloas" is the fork name. Not a typo. But "gloas" appears as "gloas" elsewhere. Consistent.

Wait: "gloas" vs "gloas"—the diff shows gloas::DataColumnSidecar. The comment says "gloas counterpart". This is consistent with codebase naming. Acceptable.

---

15. kzg_verdicts doc comment says "one per entry and in order" but doesn't specify behavior on empty input

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1352-1368

pub fn kzg_verdicts(items: &[(&AnyDataColumnSidecar, &PendingKzg)]) -> Vec<ChainVerdict> {

Problem: Empty slice returns empty vec (correct), but not documented. The short-circuit for [(single)] is an optimization leak in docs.

Recommendation: Document: "Returns an empty vec for empty input. For a single item, delegates to kzg_verdict."

---

Minor Issues

16. settle function could be #[inline]

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1320-1326

Trivial function, hot path. Mark #[inline].

---

17. ColumnRef enum derives no traits, manual From impl

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1278-1312

Could derive Clone, Copy since it holds references. Not necessary but idiomatic.

---

Test Quality

18. bench_kzg_batch_against_separate_calls is #[ignore] but uses println!

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1684-1720

Standard practice for benchmarks. Acceptable.

19. real_sidecars test helper uses expect with string literals, not unwrap_or_else

File: crates/blockchain/state_transition/src/beacon/gossip/column.rs, lines 1621-1664

In test code, expect is fine. Acceptable.

---

Summary

Priority	Count	Categories
Critical	3	Signature hashing correctness, usize overflow, unwind safety
Security	1	Unbounded memo memory
Correctness	3	Lifetime fragility, HashMap ordering, length validation
Performance	2	Unnecessary allocations, eager conversion
Style	5	Various minor improvements

The PR is well-designed and the batching optimization is valuable. The critical issues should be addressed before merge, particularly the BlsSignature hashing correctness and AssertUnwindSafe soundness claims.

---

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