fix(devnet5): don't register block-attestation data at import (broke Type-2 merge)

[MegaRedHand]

Summary

Fixes the devnet error Failed to merge Type-1s into Type-2 ... child proof deserialization failed at index N.

Root cause: block import seeded the known payload pool with TypeOneMultiSignature::empty(bits) placeholders (participant bits, empty proof bytes) so block-borne votes carried fork-choice weight. Block building read the same pool and fed those empty bytes into the Type-2 merge, where lean-multisig decompression failed.

Fix: stop registering block-attestation data in the pool at import altogether.

A first pass mirrored leanSpec's on_block — registering each attestation's data key with an empty proof set instead of a proof-less placeholder. That removed the crash, but the empty entries turned out to be inert in our weight model and introduced an unbounded-growth path (see below), so the final version drops the registration entirely.

Why removing it is safe

ethlambda derives fork-choice weight from proof participant bits, not from data-key presence:

• extract_latest_attestations iterates entry.proofs; an empty proof set yields zero voters.
• the block builder scores candidates by voters-from-proofs, so an empty entry is never selected and never reaches merge_type_1s_into_type_2.

So a registered-but-empty entry contributed zero weight and zero merge input. Its only observable effect was reserving an insertion-order slot for same-slot equivocation tie-breaking, which reaggregation re-establishes in block-attestation order anyway.

Block-borne vote weight is still recovered: reaggregation SNARK-splits the block's merged Type-2 into per-attestation Type-1s (split_type_2_by_message) into the new pool, and gossip delivers them; both migrate to the known pool at the next acceptance tick. This matches the spec's documented one-slot deferral.

Bonus: closes an unbounded-growth path

The empty-proof entries bypassed the payload buffer's only capacity limit. PayloadBuffer evicts when total_proofs > capacity, but the bare-key insertion never incremented total_proofs, so a non-finalizing chain could grow known_payloads past its bound. Removing the registration eliminates this by construction (flagged by all four PR reviewers).

Changes

• crates/blockchain/src/store.rs: on_block_core no longer registers block-attestation data keys (keeps the per-validator attestation-valid metric).
• crates/storage/src/store.rs: delete the now-dead PayloadBuffer::register_data and Store::register_known_aggregated_data_batch.
• crates/blockchain/tests/forkchoice_spectests.rs: the harness deconstructs each imported block into per-attestation Type-1s (structurally, from aggregation_bits) into the known pool and recomputes the head — reproducing the proposer-view store the fixtures encode, the role leanSpec's harness fills via its proposer-build simulation. These entries go through push, so they are properly accounted and bounded.

Notes / follow-ups

• The spec routes block-reaggregated votes to the new pool (deferred), which a leanSpec reviewer flagged as leaving receiving nodes with zero block-vote weight (Aggregated block proof - devnet5 leanEthereum/leanSpec#717, discussion_r3259006576). The new-vs-known question remains open upstream.
• Backfilling nodes don't run reaggregation (it is synced-gated in lib.rs), so block-borne votes during sync rely on gossip re-delivery. Accepted as an OK tradeoff for now; tracked as a follow-up.

Test plan

• cargo test --workspace --release — full suite passes (forkchoice 84, stf 51, signature 13, storage 38, ssz 118, …; 0 failures)
• cargo fmt --all -- --check clean
• cargo clippy --workspace --release -- -D warnings clean

[github-actions]

🤖 Kimi Code Review

Critical Consensus Considerations

1. Deferred Fork-Choice Weight (Safety/Liveness Implications)
   In crates/blockchain/src/store.rs (lines 498–528), block attestations are now registered with empty proofs via register_known_aggregated_data_batch, contributing zero weight to the fork-choice head computed during on_block. The comment explicitly acknowledges this defers weight by "up to one slot" until SNARK-splitting recovers Type-1 proofs.

   • Verify: Confirm this aligns precisely with leanSpec's on_block logic. Standard LMD GHOST expects block attestations to count immediately; deferring weight could theoretically lead to a "head oscillation" scenario if a competing block imports within the same slot before reaggregation completes. If the spec mandates this behavior, ensure the test harness in forkchoice_spectests.rs (lines 102–130) perfectly mirrors the proposer-view store assumptions to avoid false positives in spec tests.

2. Test Harness Fidelity
   crates/blockchain/tests/forkchoice_spectests.rs (lines 115–130) manually reconstructs TypeOneMultiSignature::empty entries to simulate post-import reaggregation.

   • Risk: If production SNARK-splitting produces proofs with additional metadata (or non-empty signature bytes) that affect fork-choice bookkeeping (e.g., participant bitfield interpretation), the test may pass while production fails. Ensure TypeOneMultiSignature::empty is structurally identical to the output of split_type_2_by_message.

Security Observations

3. Mutex Poisoning
   crates/storage/src/store.rs lines 1228 and 207 use lock().unwrap() on known_payloads. In a consensus client, a panic in one thread (e.g., during SSZ deserialization) could poison this lock, rendering the store unusable. Consider using parking_lot (which doesn't poison) or explicit MutexGuard error handling to prevent denial-of-service via panic propagation.

4. Buffer Capacity Check
   PayloadBuffer::register_data (lines 215–225) pushes to self.order without checking against MAX_ATTESTATIONS_DATA or any buffer limit. While on_block caps attestations at 8, repeated edge cases (e.g., duplicate data from different blocks) could grow order unbounded if pruning logic elsewhere fails. Add a defensive capacity check or assert that self.order.len() < limit before push.

Code Quality and Correctness

5. Idiomatic Rust Patterns
   • Good: Use of Vec::with_capacity in on_block_core (line 512) and into_parts() (line 216) to avoid unnecessary clones.
   • Naming: The variable known_data in on_block_core (line 512) contains HashedAttestationData, not full payloads. Consider renaming to data_keys or attestation_data to

---

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

[github-actions]

🤖 Codex Code Review

1. High: proofless known_payloads entries bypass the buffer’s only capacity limit. In crates/storage/src/store.rs, register_data() inserts a new entry without incrementing total_proofs and without any eviction path; crates/blockchain/src/store.rs now calls that for every block attestation. Since pruning only happens once an attestation target is finalized, a peer can keep feeding fresh unfinalized attestation-data roots and grow known_payloads unbounded in RAM. For consensus code, that is a straightforward memory-DoS surface. Count data-only entries toward capacity or add a separate entry-count cap/eviction path.

2. High: syncing nodes can now permanently miss block-borne fork-choice votes. crates/blockchain/src/store.rs switched block import from inserting participant-bearing pseudo-Type-1s to registering data only, then immediately recomputes head at line 526. The only recovery path is post-import reaggregation, but that is gated on synced in crates/blockchain/src/lib.rs, where synced means head_slot >= current_slot at line 496. So any block imported while backfilling never gets its attestation participants into new/known later, and its votes are absent from LMD-GHOST unless they reappear via gossip. I’d keep gossip publication gated on sync status, but still run the store update for all imported blocks.

I didn’t run Rust tests here because the environment blocks rustup from creating temp files (/home/runner/.rustup/tmp is read-only). Outside those two points, the test-only adjustments look coherent.

---

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

[github-actions]

🤖 Claude Code Review

Now I have enough context to write a thorough review.

---

PR Review: fix: drop proof-less Type-1 placeholders that broke block-proof merge

Overview

This PR fixes a crash where TypeOneMultiSignature::empty(bits) entries (participant bits, empty proof bytes) inserted at block import were fed into the Type-2 merge, causing deserialization failures. The fix correctly mirrors the spec: register only the attestation data key with an empty proof set, so the merge never sees zero-byte proof material. The test harness is updated separately to reconstruct structural Type-1s for fork-choice weight, matching the "proposer-view" store that fixtures encode.

The root-cause diagnosis is sound and the fix is logically correct.

---

Issues

1. register_data bypasses capacity enforcement (crates/storage/src/store.rs:215)

PayloadBuffer evicts entries only when total_proofs > capacity. register_data inserts into data and order without touching total_proofs, so the eviction loop in push() never fires for bare-key entries. With AGGREGATED_PAYLOAD_CAP = 512 and up to MAX_ATTESTATIONS_DATA = 8 entries per block, ~64 blocks without finalization would exceed the intended bound silently.

Bare keys do get pruned by prune() on finalization, but in a long non-finalizing chain the buffer grows unboundedly. The fix could be to count bare keys against capacity (e.g. a separate data_count field, or using total_proofs with a virtual increment of 1), or document explicitly that capacity applies only to proof slots, not data keys.

2. No unit tests for register_data / register_known_aggregated_data_batch

The new PayloadBuffer::register_data function has several non-obvious invariants (no-op when key present, no eviction trigger, order slot reserved, subsequent push for same key works correctly) and none are covered by tests. The existing test suite at line 1756+ is thorough for push and push_batch — comparable coverage for the new method would prevent regressions, especially around the capacity bypass noted above.

3. Double to_block() call in the test harness (crates/blockchain/tests/forkchoice_spectests.rs:89,117)

When block_data.block_root_label is set, to_block() is called at line 89 (for label registration) and again at line 117 (for the attestation deconstruction). If the deserialization is non-trivial, the Block from line 89 could be reused inside the if import_ok block. Minor, but easy to tighten.

4. update_head made pub instead of pub(crate) (crates/blockchain/src/store.rs:40)

Integration tests in tests/ compile as a separate crate, so pub(crate) cannot reach them — pub is technically required here. That said, making an internal bookkeeping function part of the public API of ethlambda-blockchain is worth noting. A #[doc(hidden)] attribute would at least signal it's not intended for external consumers:

#[doc(hidden)]
pub fn update_head(store: &mut Store, log_tree: bool) { ... }

5. Removed MAX_ATTESTATIONS_DATA cross-reference comment (crates/common/types/src/block.rs:126)

The deleted comment (See: leanSpec PR #717, which lowered the cap from 16 to 8...) provided useful historical context for future readers debugging cap mismatches with other clients. Worth keeping or moving into the git commit message, not silently dropping.

---

Minor observations

• The test harness correctly uses insert_known_aggregated_payloads_batch with TypeOneMultiSignature::empty(bits) entries (not register_known_aggregated_data_batch). The empty-proof TypeOneMultiSignature carries the participant bitfield that extract_latest_attestations needs to assign fork-choice weight. This intentional divergence from production is well-commented.
• register_data's no-op guard (if self.data.contains_key) correctly prevents a later bare key from overwriting a real-proof entry — the ordering is safe.
• The drain() function silently drops empty-proof entries (iterates entry.proofs which is empty). This is benign since register_data only targets known_payloads, but it means an empty-key entry in new_payloads (if ever created by future callers) would vanish on promote_new_aggregated_payloads. Worth a comment in drain() if the invariant is intentional.

---

Summary

The core fix is correct and the spec alignment is well-justified. The most actionable items before merge are:

• Item 1 (capacity bypass) is a latent correctness issue that could surface in stress scenarios or non-finalizing chains.
• Item 2 (missing tests) is a coverage gap for new code with subtle semantics.
• Items 3–5 are polish.

---

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

[greptile-apps]

Greptile Summary

This PR fixes a Type-2 merge crash on devnet5 by removing the registration of block-attestation data into the known payload pool during block import. The root cause was that on_block_core seeded the pool with TypeOneMultiSignature::empty placeholders whose empty proof bytes were later fed into the merge_type_1s_into_type_2 path, causing lean-multisig decompression to fail.

• crates/blockchain/src/store.rs: on_block_core no longer inserts empty-proof Type-1 placeholders for block attestations into the known pool; block-borne votes are now recovered exclusively through the reaggregation/gossip path, matching the one-slot deferral documented in leanSpec. update_head is made pub to support the updated test harness.
• crates/blockchain/tests/forkchoice_spectests.rs: The fork-choice spectest harness now structurally deconstructs each successfully imported block into per-attestation empty-proof Type-1s and inserts them directly into the known pool, then recomputes the head — reproducing the proposer-view store that the fixtures encode without needing a real SNARK split.
• crates/common/types/src/block.rs: Removes a stale reference to leanSpec PR #717 from the MAX_ATTESTATIONS_DATA doc comment.

Confidence Score: 5/5

Safe to merge — the change removes a clearly broken code path and the replacement design is thoroughly reasoned.

The deletion of the empty-proof pool seeding directly eliminates the crash site. The safety analysis in the PR description is rigorous: fork-choice weight derives from participant bits (not bare key presence), empty entries contributed zero merge input, and block-borne votes are still recovered through reaggregation/gossip within one slot. The test harness update correctly replicates the proposer-view store that fixtures require, going through push so eviction accounting remains correct.

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Removes the empty-proof pool seeding from on_block_core and exposes update_head as pub; both changes are correct and directly fix the Type-2 merge crash.
crates/blockchain/tests/forkchoice_spectests.rs	Test harness updated to structurally reconstruct per-attestation Type-1 entries from block body after a successful import and recompute the head, correctly reproducing the proposer-view store required by fixtures.
crates/common/types/src/block.rs	Removes a stale leanSpec PR reference from the MAX_ATTESTATIONS_DATA doc comment; no functional change.

Sequence Diagram

sequenceDiagram
    participant Net as Network
    participant BC as on_block_core
    participant KP as known_payloads
    participant NP as new_payloads
    participant FC as update_head

    Note over BC,FC: Before this PR (broken path)
    Net->>BC: SignedBlock (with attestations)
    BC->>KP: insert empty-proof TypeOneMultiSig per attestation
    BC->>FC: update_head()
    FC->>KP: extract_latest_known_attestations()
    Note over KP: empty proof bytes fed into merge_type_1s_into_type_2 CRASH

    Note over BC,FC: After this PR (fixed path)
    Net->>BC: SignedBlock (with attestations)
    BC->>BC: count metrics only (no pool insert)
    BC->>FC: update_head() block votes deferred
    FC->>KP: extract_latest_known_attestations()
    Note over KP,NP: Block votes recovered via reaggregation / gossip
    Net-->>NP: gossip Type-1s or split_type_2_by_message
    NP-->>KP: promote at next acceptance tick
    FC->>KP: extract_latest_known_attestations() next slot

Loading

_{Reviews (2): Last reviewed commit: "docs: remove comment" | Re-trigger Greptile}

[github-actions]

🤖 Kimi Code Review

Critical Consensus Issue

crates/blockchain/src/store.rs (lines 498–508): The removal of insert_known_aggregated_payloads_batch from on_block_core eliminates fork-choice visibility of block-borne attestations in the production code path. The test code (lines 114–131 of forkchoice_spectests.rs) explicitly compensates for this by manually injecting attestations and re-running update_head, citing that "the real node SNARK-splits the block's merged Type-2 proof."

If the SNARK-splitting logic is not yet implemented in the production import flow, this PR introduces a consensus-critical bug where imported block attestations are ignored by LMD GHOST, causing nodes to potentially settle on different heads than the spec expects. Verify that split_type_2_by_message (or equivalent) is invoked during production block processing to repopulate the attestation pool before this removal ships.

Code Quality & Safety

1. crates/blockchain/src/store.rs:40 – Visibility change to pub exposes fork-choice internals. If this is solely for the test file, prefer pub(crate) or #[cfg(test)] pub to minimize the public API surface and prevent external misuse of head-update logic.

2. crates/blockchain/tests/forkchoice_spectests.rs:114–131 – The double update_head call (once inside on_block_without_verification, once manually after attestation injection) is correct for matching the proposer-view semantics of the fixtures, but add a comment explaining that the second call is test-harness-specific to prevent future maintainers from assuming production requires this pattern.

3. crates/blockchain/src/store.rs:495–501 – The metrics counting logic is preserved, which is good for observability. Ensure that validator_indices (used in the remaining loop) handles the bitfield correctly and cannot panic on malformed inputs; otherwise, a malicious block could crash the node.

Documentation

crates/common/types/src/block.rs (removed lines): The deletion of the PR reference comment is acceptable for cleanliness, but ensure the constant MAX_ATTESTATIONS_DATA still has sufficient context about why the limit exists (consensus spec reference).

Recommendations

1. Block this PR until you confirm that SNARK-splitting recovery of Type-1 attestations is implemented and invoked in the production on_block flow (not just in tests). If that logic lives in a separate crate or future PR, keep the current insertion logic with a TODO until the replacement is wired in.
2. If update_head must remain public, document the precondition that the store must be in a consistent state (no incomplete batches) to avoid race conditions during head computation.
3. Consider asserting in the test that insert_known_aggregated_payloads_batch actually changes the head root when votes are inserted, ensuring the fork-choice integration is functional.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. Block import no longer feeds block-body attestations into production fork choice, which is a consensus regression. In store.rs, the old insert_known_aggregated_payloads_batch(...) path was removed, but update_head() still runs immediately afterward at store.rs. For locally proposed blocks, propose_block() only calls self.process_block(...) at lib.rs, so their included attestations now never enter either payload buffer unless the same block later comes back from the network. That means LMD-GHOST can compute the next head from stale votes, which is especially dangerous around close forks and proposer timing.

2. Received blocks are also handled inconsistently now, and the new test code masks the bug instead of exercising the runtime path. Network import only runs reaggregation when synced at lib.rs, and reaggregation inserts into the new pool only at reaggregate.rs, without promoting to known or recomputing head. So even synced nodes delay block-borne vote weight until a later acceptance tick, while syncing nodes drop it entirely. The new spectest shim then manually inserts empty proofs into known and calls update_head() at forkchoice_spectests.rs, which makes tests pass for a state the real node never reaches.

Notes

I did not find additional issues in the small diff beyond that fork-choice regression. I also could not run cargo test in this environment because rustup failed to create temp files under /home/runner/.rustup/tmp (read-only filesystem).

---

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

[github-actions]

🤖 Claude Code Review

Now I have all the context I need. Here is my review:

---

Code Review: fix(devnet5): don't register block-attestation data at import

Overview

This PR fixes a devnet crash (Failed to merge Type-1s into Type-2 ... child proof deserialization failed at index N) by removing the block-import code that seeded the known payload pool with TypeOneMultiSignature::empty(bits) placeholders. These empty-proof entries were safe for fork-choice weight (zero participants, zero votes) but broke the block builder when it tried to deserialize their empty bytes as Type-1 proof material for a Type-2 merge. The fix removes the production registration entirely and replicates the equivalent functionality in the test harness only.

The root-cause analysis in the PR description is accurate and well-reasoned.

---

Changes by File

crates/blockchain/src/store.rs

• The removal of the registration loop and the known_entries batch insertion is correct and safe. The justification (zero weight from empty proof sets, deferred recovery via reaggregation/gossip) is sound.
• The per-attestation metrics::inc_attestations_valid(count) call is correctly preserved in the simplified loop.
• Concern — pub fn update_head: The function was previously fn (module-private). Making it pub is the only viable option to call it from integration tests in tests/, but it now permanently exposes an internal fork-choice trigger as a public API. External callers could invoke it at incorrect times (e.g., while the store is mid-update). Consider wrapping the test-only two-step operation (insert_known_aggregated_payloads_batch + update_head) in a dedicated pub fn inject_block_attestations in the store module, with a doc comment that this is intended for test harnesses. That keeps the primitive private while making the test intent explicit.

crates/blockchain/tests/forkchoice_spectests.rs

• The structural reconstruction of per-attestation Type-1s from aggregation_bits is a sound substitute for SNARK-splitting when proof bytes are irrelevant to fork-choice weight.
• The second update_head call after injecting synthetic entries is correctly reasoned: on_block_core runs the first head update before the block's own attestation votes exist in the pool; the second call catches them.
• Test divergence from production path: The harness inserts synthetic entries directly into the known pool. In production, equivalent entries enter via the new pool and are promoted one tick later (the open question referenced in leanSpec#717). Tests will pass even if the new→known promotion path for block-borne votes has a bug, because the harness bypasses it. This is an accepted limitation noted in the PR, but worth documenting as a test invariant so future readers understand why insert_known_aggregated_payloads_batch is used rather than insert_new_aggregated_payloads_batch.
• Minor — double deserialization: For labeled block steps, block_data.to_block() is called once at line 89 (label registration) and again at line 117 (attestation extraction). The Block could be computed once and reused for both purposes, though this is test-only code and has no correctness impact.

crates/common/types/src/block.rs

• Removal of the leanSpec PR #717 reference from the MAX_ATTESTATIONS_DATA doc comment is fine. That context belongs in commit history, not in the constant's doc.

PR description vs. diff discrepancy: The description says "crates/storage/src/store.rs: delete the now-dead PayloadBuffer::register_data and Store::register_known_aggregated_data_batch", but neither function appears in the diff and neither exists in the current storage/src/store.rs. The description may refer to functions removed in a prior commit or functions that were never added. Not a blocking issue, but the description should be corrected.

---

Correctness of the Capacity-Bound Fix

The PR correctly notes that the old code bypassed PayloadBuffer's capacity limit. push() increments total_proofs only when a proof is added; a bare data-key insertion with no proof (the old register_data pattern) never incremented total_proofs, so eviction never triggered for those entries. Removing the registration entirely closes this unbounded-growth path by construction. This is a meaningful correctness improvement beyond the crash fix.

---

Summary

• The crash fix is correct and the safety argument is well-supported.
• The capacity-bypass fix is a real correctness improvement.
• Two things to address before merging:
  1. pub fn update_head — consider a named wrapper (e.g., pub fn inject_block_attestations_for_testing) instead of exposing the raw head-update primitive.
  2. Add a brief comment in the test harness explaining why insert_known_aggregated_payloads_batch is used instead of the production new-pool path, so future readers don't "fix" it toward spec and break the fixtures.
• The PR description's mention of storage/src/store.rs changes doesn't match the actual diff — clarify or remove that bullet.

---

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