feat(blockchain): gate proposer attestation aggregation behind a flag

[MegaRedHand]

Stacked on #477 (refactor/cli-options-module). Base will retarget to main once #477 merges.

What

Adds --enable-proposer-aggregation (default off) controlling how build_block collapses attestations that share the same AttestationData.

A block may carry at most one entry per AttestationData — on_block rejects duplicates (StoreError::DuplicateAttestationData) before signature verification, so the proposer must collapse same-data proofs to one either way. This flag only changes how:

	strategy	coverage	cost
enabled	merge same-data proofs via recursive Type-1 aggregation into one union-coverage proof (leanSpec #510)	max (union of all proofs)	one leanVM aggregation per duplicated data entry
disabled (default)	keep the single best-coverage proof per data, drop the rest	bounded by the best individual proof	no leanVM aggregation

Today the proposer always aggregates; the dominant cost of building blocks that carry attestations is exactly this per-data leanVM aggregation. The flag makes it opt-in.

Threading

CliOptions.enable_proposer_aggregation   (bin/ethlambda/src/cli.rs, default false)
  -> BlockChain::spawn(..)                (crates/blockchain/src/lib.rs)
    -> BlockChainServer.enable_proposer_aggregation
      -> produce_block_with_signatures(.., flag)   (store.rs)
        -> build_block(.., flag)          (block_builder.rs)
           ├─ enabled  -> compact_attestations (unchanged)
           └─ disabled -> keep_best_proof_per_data (new)

keep_best_proof_per_data groups selected entries by AttestationData, keeps the entry with the most participants (ties → first occurrence), and preserves first-occurrence order — mirroring compact_attestations' grouping but with no crypto. The one-entry-per-data invariant and the 1:1 attestation↔proof correspondence hold in both paths.

Behavior change

The binary default flips: proposers stop merging same-data proofs unless --enable-proposer-aggregation is passed. Blocks stay valid and interoperable; attestation entries just carry less voter coverage (the best single proof rather than the union).

Scope note

This gates only the in-build_block collapse (per the request). The separate Type-1→Type-2 merge in propose_block is untouched. With aggregation off there are at most MAX_ATTESTATIONS_DATA single-proof components feeding that merge, same as before (one per data) — so no blow-up there. Net build-time impact is worth a devnet A/B.

Tests

• New build_block_without_proposer_aggregation_keeps_single_best_proof_per_data: one AttestationData with two proofs ({0} and {1,2}); with the flag off, asserts exactly one entry survives, 1:1 with its signature, and the kept proof is the higher-coverage {1,2} (no leanVM crypto invoked).
• Existing build_block tests pass true to preserve their current (aggregating) behavior.
• cargo clippy -p ethlambda-blockchain -p ethlambda --all-targets -- -D warnings ✅
• cargo test -p ethlambda-blockchain --lib ✅ (39 passed)

History

First commit took the naive "leave duplicates unmerged" approach; the follow-up fix (keep_best_proof_per_data) corrects it after confirming on_block rejects duplicate AttestationData.

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/src/block_builder.rs:88, bin/ethlambda/src/cli.rs:94, and crates/blockchain/src/block_builder.rs:967 introduce a proposer mode that can build blocks this node will reject itself. When enable_proposer_aggregation is false, build_block leaves multiple entries with identical AttestationData in the block body. But on_block explicitly rejects any duplicate AttestationData (crates/blockchain/src/store.rs:539), and the proposer imports its freshly built block before publishing it (crates/blockchain/src/lib.rs:645). So the new default path can turn a valid attestation pool state into a missed proposal whenever extend_proofs_greedily selects more than one proof for the same data root. The new test is also asserting the opposite of the store invariant. I would treat this as a correctness blocker: either keep compaction unconditional, or remove/relax the duplicate-data rejection consistently across import/verification/state-transition paths and add an end-to-end test covering produce_block_with_signatures through local import.

I did not find a second issue in the diff. I couldn’t run cargo test in this environment because the cargo wrapper tried to write under /home/runner/.rustup, which is read-only here.

---

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

[github-actions]

🤖 Kimi Code Review

Overall Assessment: The PR correctly implements a feature flag for proposer-side attestation aggregation with proper documentation, parameter propagation, and test coverage. No security vulnerabilities or consensus-critical bugs detected.

Specific Feedback

1. Test Maintainability (Minor)

File: crates/blockchain/src/block_builder.rs
Lines: 958–1050 (new test)

The new test duplicates ~60 lines of setup boilerplate (validator generation, state initialization, checkpoint construction) from existing tests like build_block_includes_gap_closing_attestations.

Suggestion: Extract a helper function fn setup_test_state(num_validators: usize, head_slot: u64) -> (State, Vec<H256>, H256) to reduce duplication and improve maintainability. This follows Rust best practices for test hygiene.

2. Documentation Clarity

File: crates/blockchain/src/block_builder.rs
Line: 50–59

The doc comment correctly notes that both aggregated and non-aggregated forms are valid per consensus rules. Consider explicitly stating that this affects only the block's attestations list size, not the state transition's vote counting logic (which you mention but could emphasize).

3. Parameter Propagation Verification

File: crates/blockchain/src/lib.rs
Lines: 502–510

Correctly threads self.enable_proposer_aggregation from the server state into produce_block_with_signatures. The parameter flow is sound:

1. CLI flag → main.rs (line 218)
2. BlockChain::spawn() → stored in BlockChainServer (line 110)
3. produce_block_with_signatures() call (line 508)

4. Consensus Safety Check

File: crates/blockchain/src/block_builder.rs
Line: 88–92

The conditional logic correctly preserves the 1:1 correspondence between AttestationData and TypeOneMultiSignature when aggregation is disabled. This maintains the invariant required by the state transition function that processes attestations.

Note: The test at line 1009 uses TypeOneMultiSignature::empty() which is appropriate since the test validates structural correctness (entry counts) rather than cryptographic validity. Ensure your XMSS verification in process_block handles empty proofs gracefully when this flag is disabled in production (though this appears to be a test-only utility).

5. Performance Considerations

The change correctly avoids the compact_attestations leanVM call when disabled (line 88–92), saving significant CPU during block production. The metrics observation at line 96 will show near-zero time for the "compact" phase when disabled, which is useful for monitoring.

6. Rust Idioms

File: crates/blockchain/src/store.rs
Line: 789–793

The function signature extension maintains consistency with existing parameters. Consider whether enable_proposer_aggregation should be bundled into a BlockBuildConfig struct if more flags are added later, but for now the flat parameter is idiomatic and clear.

Conclusion: LGTM. Address the test helper extraction in a follow-up if desired, but not blocking.

---

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

[greptile-apps]

Greptile Summary

This PR makes proposer-side attestation aggregation opt-in. The main changes are:

• Added the --enable-proposer-aggregation CLI flag.
• Threaded the flag through block production.
• Gated build_block attestation compaction behind the flag.
• Added a test for the unmerged duplicate-data path.

Confidence Score: 4/5

The default proposal path can build a block that local import rejects before gossip.

• Skipping compaction can leave duplicate AttestationData entries in the block body.
• The local publish path still rejects duplicate AttestationData during self-import.
• Flag parsing and propagation look consistent.

crates/blockchain/src/block_builder.rs and the duplicate-attestation check in crates/blockchain/src/store.rs

Important Files Changed

Filename	Overview
bin/ethlambda/src/cli.rs	Adds the opt-in proposer aggregation flag with a false default.
bin/ethlambda/src/main.rs	Passes the new CLI flag into blockchain startup.
crates/blockchain/src/lib.rs	Stores the proposer aggregation setting and forwards it into block production.
crates/blockchain/src/store.rs	Threads the flag into build_block while retaining duplicate-data rejection on import.
crates/blockchain/src/block_builder.rs	Skips compaction when the flag is off, which can produce duplicate-data blocks rejected by the publish path.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/blockchain/src/block_builder.rs:90-92
**Duplicate Attestations Reject Proposals**

When aggregation is disabled, this branch keeps multiple selected proofs with the same `AttestationData` as separate block-body entries. The normal proposer path self-imports the built block before gossiping it, and `on_block_core` rejects duplicate `AttestationData`, so a proposer can miss its slot whenever the pool has two useful proofs for one data root.

_{Reviews (1): Last reviewed commit: "feat(blockchain): gate proposer attestat..." | Re-trigger Greptile}

[greptile-apps]

Duplicate Attestations Reject Proposals

When aggregation is disabled, this branch keeps multiple selected proofs with the same AttestationData as separate block-body entries. The normal proposer path self-imports the built block before gossiping it, and on_block_core rejects duplicate AttestationData, so a proposer can miss its slot whenever the pool has two useful proofs for one data root.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/blockchain/src/block_builder.rs
Line: 90-92

Comment:
**Duplicate Attestations Reject Proposals**

When aggregation is disabled, this branch keeps multiple selected proofs with the same `AttestationData` as separate block-body entries. The normal proposer path self-imports the built block before gossiping it, and `on_block_core` rejects duplicate `AttestationData`, so a proposer can miss its slot whenever the pool has two useful proofs for one data root.

How can I resolve this? If you propose a fix, please make it concise.

[github-actions]

🤖 Claude Code Review

Now I have verified findings. Let me compose the final review.

---

Review: feat(blockchain): gate proposer attestation aggregation behind a flag

This PR adds --enable-proposer-aggregation (default off), threading a bool flag from the CLI through BlockChain::spawn → BlockChainServer → produce_block_with_signatures → build_block, where it gates the compact_attestations call. The flag propagation is mechanically clean — all call sites were found and updated correctly, with appropriate test coverage for the new non-aggregating path.

---

Finding 1 — Misleading compact-phase metric recorded when no compaction runs

crates/blockchain/src/block_builder.rs (~line 87–93)

compact_start is started and metrics::observe_block_proposal_phase("compact", compact_start.elapsed()) fires unconditionally after the if/else, regardless of whether any compaction occurred:

let compact_start = Instant::now();
let compacted = if enable_proposer_aggregation {
    compact_attestations(selected, head_state)?
} else {
    selected   // no work done — but timer still runs
};
metrics::observe_block_proposal_phase("compact", compact_start.elapsed());  // ← always fires

With enable_proposer_aggregation = false (the new default), every block proposal records a spurious nanosecond-level sample into lean_block_proposal_attestation_build_phase_seconds{phase="compact"}. That bucket is documented as timing "recursive merge of proofs sharing the same AttestationData" — work that is entirely skipped. On a busy node this inflates the histogram's sample count with noise, suppressing real p95/p99 latency signals when the flag is later toggled on.

Fix: Move the timer start and observe inside the if branch only, or add a "compact_skipped" label when the flag is off.

---

Finding 2 — Comment blocks explain mechanics (WHAT), not constraints (WHY)

crates/blockchain/src/block_builder.rs (~lines 53–59 and 83–86), crates/blockchain/src/lib.rs (~lines 171–176)

CLAUDE.md: "Default to writing no comments. Only add one when the WHY is non-obvious: a hidden constraint, a subtle invariant, a workaround for a specific bug, behavior that would surprise a reader."

Three added blocks restate in prose exactly what the code and parameter name already say:

• The inline comment at lines 83–86 ("Gated by enable_proposer_aggregation: when disabled, the selected entries are used unmerged...") just paraphrases the if/else below it.
• The BlockChainServer field docstring (lib.rs ~lines 171–176) repeats the CLI flag description word-for-word.
• The build_block docstring paragraph added at lines 53–59 is five lines of mechanics where one line carries the only non-obvious invariant: "Both forms are valid; the state transition unions votes by target root."

That one-liner is worth keeping — it's the subtle justification that justifies the whole design. The surrounding mechanical description should be cut.

---

Finding 3 — Static node policy threaded through two function signatures

crates/blockchain/src/store.rs:792, crates/blockchain/src/block_builder.rs:67

enable_proposer_aggregation is a node-wide configuration set once at startup and never updated at runtime (unlike aggregator, which has AggregatorController with a hot-toggle API). Despite being static config, it is plumbed explicitly through produce_block_with_signatures and build_block, functions whose other parameters are all block-content inputs (slot, validator_index, head_state, aggregated_payloads, etc.).

This is a small scope PR so the current approach is defensible, but each new policy flag added this way widens two unrelated function signatures. The cleaner altitude for static node policy would be a config struct at the BlockChainServer level (where other node-wide booleans such as gate_duties already live) rather than a bare parameter threaded into the block-building leaf functions. Worth a note for the next feature flag.

---

Summary

#	Severity	Status
1	Moderate — observability	CONFIRMED — act on
2	Low — maintainability	CONFIRMED — trim the comments
3	Low — design	PLAUSIBLE — worth a note

The correctness concern raised during review (blocks exceeding MAX_ATTESTATIONS_DATA) was refuted: select_attestations bounds distinct data rounds by MAX_ATTESTATIONS_DATA, not total proof entries — multiple proofs per data root are intentional and valid per the spec. The SSZ list try_into().expect() is the hard capacity guard.

---

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