Skip to content

refactor(types): port leanSpec Type-1 / Type-2 aggregation envelope#361

Merged
MegaRedHand merged 8 commits into
devnet5from
type1-type2-aggregation
May 12, 2026
Merged

refactor(types): port leanSpec Type-1 / Type-2 aggregation envelope#361
MegaRedHand merged 8 commits into
devnet5from
type1-type2-aggregation

Conversation

@pablodeymo
Copy link
Copy Markdown
Collaborator

🗒️ Description / Motivation

Ports the typed two-level multi-signature envelope introduced by contributor commit
anshalshukla/leanSpec@0ab09dd
("dummy type 1 and type 2 aggregation with block proofs") to ethlambda:

  • TypeOneMultiSignature — single-message N-signer proof; replaces AggregatedSignatureProof on the SignedAggregatedAttestation gossip wire.
  • TypeTwoMultiSignature — merged multi-message proof binding every per-attestation Type-1 plus a singleton proposer Type-1 over the block root.
  • SignedBlock.signature: BlockSignaturesSignedBlock.proof: ByteListMiB carrying the SSZ-encoded merged Type-2.

The upstream commit is WIP (verify functions are explicit stubs, not yet in canonical leanethereum/leanSpec). ethlambda leads the wire-shape migration so the type plumbing is in place when canonical absorbs the refactor and real lean_multisig bindings land. Opening as draft until canonical catches up.

What Changed

Landed as three commits, one per phase. Each phase compiled and passed make test independently.

Phase 1 — f2d0fb5 — additive type plumbing

  • crates/common/types/src/block.rs — added TypeOneInfo, TypeOneMultiSignature, TypeOneInfos (SSZ-list limit MAX_ATTESTATIONS_DATA + 1), TypeTwoMultiSignature, and BytecodeClaim (typed alias for H256, placeholder until lean_multisig defines the trusted evaluation).
  • SSZ round-trip + capacity unit tests.
  • Pure additive: no consumers yet.

Phase 2 — 18a60b5 — gossip-layer pipeline

  • crates/common/types/src/attestation.rsSignedAggregatedAttestation.proofTypeOneMultiSignature.
  • crates/blockchain/src/aggregation.rsAggregatedGroupOutput.proof, aggregate_job, resolve_child_pubkeys, select_proofs_greedily all carry/read Type-1.
  • crates/storage/src/store.rsPayloadEntry.proofs: Vec<TypeOneMultiSignature>; subsumption logic reads info.participants.
  • Block-builder helpers (compact_attestations, extend_proofs_greedily, build_block) operate on Type-1 throughout.
  • Temporary to_legacy / from_legacy boundary at block assembly + block-body ingestion so SignedBlock wire stayed legacy through Phase 2.

Phase 3 — fc9ce1f — block wire + storage

  • SignedBlock.signature: BlockSignaturesSignedBlock.proof: ByteListMiB. Legacy BlockSignatures / AttestationSignatures / AggregatedSignatureProof removed.
  • crates/blockchain/src/lib.rs::propose_block wraps the proposer XMSS as a singleton Type-1, calls aggregate_type_2, SSZ-encodes the merged proof, and stashes it on SignedBlock.proof.
  • crates/blockchain/src/store.rs::verify_signatures rewritten as a structural-only check (mirrors upstream verify_type_2 stub): decode the merged proof, assert info.len() == attestations.len() + 1, validate per-attestation (message, slot, participants) alignment and the trailing proposer entry; no per-Type-1 crypto.
  • crates/storage/src/store.rs::write_signed_block / get_signed_block now store ByteListMiB blobs in the existing BlockSignatures column family (renaming deferred to avoid a CF migration).
  • aggregate_type_2 is a no-crypto stub today: it preserves the full TypeOneInfos metadata list but leaves proof: ByteListMiB::default(). Real merging arrives when lean_multisig exposes a merged-proof primitive — the existing aggregate_proofs only handles single-message merging.
  • Test fixtures regenerated from canonical leanSpec (make leanSpec/fixtures). The regen also cleared three pre-existing forkchoice spec failures on main (AttestationTooFarInFuture ×2, AggregateVerificationFailed(InvalidProof) on test_valid_gossip_aggregated_attestation) — they were stale-fixture artifacts.

Correctness / Behavior Guarantees

Verified at gossip: on_gossip_aggregated_attestation continues to run real ethlambda_crypto::verify_aggregated_signature on every SignedAggregatedAttestation. Invalid aggregates are rejected at the gossip boundary just like before.

Block-level becomes structural: Block-level verification no longer crypto-verifies the merged proof. The merged proof bytes can't be split client-side (the type-2 merging primitive doesn't exist in lean-multisig yet — the existing aggregate_proofs is single-message only). verify_signatures enforces:

  • info.len() == attestations.len() + 1,
  • each info[i] matches the corresponding block.body.attestations[i] on participants, slot, and message,
  • the trailing info[N] has message == block_root, slot == block.slot, single-bit participants set to block.proposer_index,
  • all participant indices fit within the validator registry.

This is the conscious "mirror upstream stubs" trade-off agreed during planning. When lean_multisig ships a real verify_type_2, the structural stub is swapped for the real call.

Block-body ingestion preserves fork-choice LMD GHOST inputs: since the merged proof can't be split, process_new_block inserts info-only Type-1 entries (real (message, slot, participants), empty proof bytes) into the payload buffer. extract_latest_known_attestations works unchanged. Empty-bytes entries never get fed back into aggregate_proofs (that path is only hit when multiple proofs share the same AttestationData, in which case at least one came from gossip with real bytes).

Storage: Table name kept (BlockSignatures) to avoid a RocksDB CF migration; doc comment updated. Renaming to Table::BlockProof is a follow-up.

Skipped tests, all behind TODO(type1-type2):

  • ssz_spectests.rs: SignedBlock, BlockSignatures, AggregatedSignatureProof, SignedAggregatedAttestation — on-disk SSZ bytes still use the legacy schema since canonical leanSpec hasn't absorbed the refactor.
  • signature_spectests.rs: test_invalid_proposer_signature — relies on block-level proposer-signature crypto, which is now a structural stub.

Attempted to bump LEAN_SPEC_COMMIT_HASH to anshalshukla/leanSpec@0ab09dd to regenerate fixtures against the new schema. Reverted: the upstream testing harness in that commit (leanSpec/packages/testing/src/consensus_testing/keys.py) still imports AttestationSignatures, which the same commit removes — fill crashes on module load. Documented in a NOTE(type1-type2) in the Makefile.

Tests Added / Run

  • Added: SSZ round-trip and capacity unit tests for the new Type-1/Type-2 containers in crates/common/types/src/block.rs.
  • Updated: verify_signatures_rejects_participants_mismatch, build_block_caps_attestation_data_entries, on_block_rejects_duplicate_attestation_data, the compact_attestations and extend_proofs_greedily tests, all forkchoice_spectests.rs step builders, signature_types.rs fixture converter, and the rpc::test_get_latest_finalized_block test — all rebuilt to construct the new merged-proof shape.
  • Verified locally:
    • make fmt — clean
    • cargo clippy --workspace --all-targets -- -D warnings — clean
    • cargo test --workspace --release — green (84 forkchoice spec tests, 7 signature spec tests with 1 expected skip, all unit tests pass)

Related Issues / PRs

  • Upstream commit being ported: anshalshukla/leanSpec@0ab09dd
  • Follow-ups when canonical absorbs the refactor:
    • Swap the structural verify_type_2 stub for the real lean_multisig primitive.
    • Revert LEAN_SPEC_COMMIT_HASH skip markers in ssz_spectests.rs and signature_spectests.rs.
    • Consider renaming Table::BlockSignaturesTable::BlockProof.

✅ Verification Checklist

  • Ran make fmt — clean
  • Ran make lint (clippy with -D warnings) — clean
  • Ran cargo test --workspace --release — all passing

…nature, and the BytecodeClaim placeholder to ethlambda-types, mirroring leanSpec commit

  anshalshukla/leanSpec@0ab09dd (dummy type 1 and type 2 aggregation with block proofs). Additive change with SSZ round-trip and capacity unit tests; the new types coexist with the
  legacy AggregatedSignatureProof / BlockSignatures wire shape until later phases migrate consumers.
…-signature envelope: SignedAggregatedAttestation.proof, AggregatedGroupOutput.proof, the storage PayloadBuffer, and

  the block-building helpers (compact_attestations, extend_proofs_greedily, build_block) now all carry TypeOneMultiSignature, reading participants from info.participants. SignedBlock
  still uses the legacy BlockSignatures shape on the wire; a temporary to_legacy/from_legacy boundary converts at block assembly and at block-body ingestion until Phase 3 swaps
  SignedBlock to a single Type-2 merged proof.
…stMiB carrying the SSZ-encoded TypeTwoMultiSignature that merges every per-attestation Type-1 plus a singleton

  proposer Type-1, completing the Type-1/Type-2 aggregation migration end-to-end. Block-level signature verification becomes a structural-only check (mirrors upstream's verify_type_2
  stub) and gossip-time per-attestation verify remains the real safety net; block-body ingestion now decodes the merged proof, asserts info.len() == attestations.len() + 1, and feeds
  info-only Type-1 entries into the payload buffer for fork choice. Storage writes the proof blob into the existing BlockSignatures column family unchanged; legacy BlockSignatures /
  AttestationSignatures / AggregatedSignatureProof types are removed. SSZ-spec cases for the affected containers and one signature-spec case that relied on proposer-signature crypto are
  gated behind TODO(type1-type2) skips pending real verify_type_2 bindings. Attempted bump to anshalshukla/leanSpec@0ab09dd reverted (its testing harness still imports the removed
  AttestationSignatures so fixtures don't generate); pinned to canonical with a NOTE and regenerated fixtures.
…pe2-aggregation. PR #357 moved the per-test-binary fixture types (signature_types.rs, common.rs, build_signed_block)

   into the shared ethlambda-test-fixtures crate and renamed verify_signatures → verify_block_signatures (made pub) so the new RPC test-driver can call it. Conflicts resolved by: taking
  the rename and keeping our structural-only verify body; deleting the now-duplicated test/signature_types.rs and test/forkchoice_spectests.rs::build_signed_block; promoting our
  aggregate_type_2 / proposer_type_one helpers to TypeTwoMultiSignature::from_type_1s and TypeOneMultiSignature::for_proposer on ethlambda-types so the test-fixtures crate can build the
  merged Type-2 shape without depending on the blockchain layer; rewriting to_blank_signed_block (fork_choice) and From<TestSignedBlock> + try_into_signed_block_with_proofs
  (verify_signatures) in the test-fixtures crate to emit Type-2 proofs; updating test_driver's gossipAggregatedAttestation handler to build TypeOneMultiSignature instead of the removed
  AggregatedSignatureProof. cargo clippy --workspace --all-targets -- -D warnings and cargo test --workspace --release both green.
Comment thread crates/blockchain/src/aggregation.rs Outdated
/// `verify_type_2` primitive.
///
/// TODO(type1-type2): re-enable once block-level crypto verification returns.
const SKIP_TESTS: &[&str] = &["test_invalid_proposer_signature"];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to review why this fails

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phase 3 replaced per-attestation verify_aggregated_signature + standalone proposer-signature crypto at block import with a structural check on the merged Type-2 envelope. The skipped test_invalid_proposer_signature flips proposer signature bytes and expects rejection on the crypto leg — that leg moved to gossip-time per-attestation verification, so the test passes against the structural stub and is misleading as-is. Re-enabled in the same follow-up that swaps the structural stub for the real verify_type_2 call (see thread on block.rs:178).

Comment thread crates/common/test-fixtures/src/fork_choice.rs Outdated
Comment on lines +171 to +178
/// Merge a list of Type-1 single-message proofs into a single Type-2
/// multi-message proof. Mirrors upstream leanSpec's `aggregate_type_2`
/// stub: the metadata list (`TypeOneInfos`) is faithfully preserved so a
/// verifier can re-derive the per-message binding inputs, but the merged
/// `proof` bytes are left empty until the `lean_multisig_py` bindings ship
/// real cryptographic merging. Block-level signature verification stays
/// structural-only in the meantime, and per-attestation crypto verification
/// continues to run at gossip ingestion.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we still lack the crypto primitives for this. Can we try using the devnet5 branch in leanMultisig?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — checked leanethereum/leanMultisig devnet5. It re-exports everything we need from rec_aggregation: TypeOneInfo, TypeOneMultiSignature, TypeTwoMultiSignature, aggregate_type_1, merge_many_type_1, split_type_2, verify_type_1, verify_type_2 (see src/lib.rs).

Two integration considerations for the follow-up PR:

  • Their TypeTwoMultiSignature uses postcard + lz4 (not SSZ) and carries ExecutionProof + Evaluation<EF>, so we keep our outer SSZ proof: ByteListMiB and stuff the compress() bytes inside.
  • setup_prover() / setup_verifier() must run once at boot.

Tracking this as a follow-up rather than expanding the scope of this PR — OK to keep the structural stub here in the meantime?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Let's integrate in another PR

Comment on lines +127 to +134
// NOTE: After Phase 3 the legacy `BlockSignatures` / `AttestationSignatures` /
// `AggregatedSignatureProof` containers are removed from the domain, and
// `SignedBlock` now carries a single `proof: ByteListMiB` field. The pinned
// leanSpec fixtures still use the old shape, so SSZ-byte and root assertions
// for `SignedBlock`, `BlockSignatures`, `AggregatedSignatureProof`, and
// `SignedAggregatedAttestation` are intentionally skipped in
// `ssz_spectests.rs::run_ssz_test` until the fixture commit is bumped to the
// Type-1/Type-2 schema.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not remove the types yet? It sounds like this is only temporary

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporariness is real, but I think the right cure is wiring up lean-multisig devnet5 (see thread on block.rs:178) rather than keeping the legacy containers around. Reasons:

  • The legacy BlockSignatures / AttestationSignatures / AggregatedSignatureProof shape is incompatible with how the merged Type-2 proof actually works on the wire — they assume per-attestation signatures travel inside the block, while Type-2 collapses them into a single merged blob, so a compat shim would be a no-op.
  • Keeping dead types invites accidental use of the wrong shape during the migration.
  • The follow-up PR is small once the dep is in: mostly the from_type_1s and verify_block_signatures swaps.

Happy to revisit if you want a belt-and-braces approach.

@MegaRedHand MegaRedHand changed the base branch from main to devnet5 May 12, 2026 14:53
@MegaRedHand MegaRedHand marked this pull request as ready for review May 12, 2026 17:54
@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

Here is the review of the Type-1/Type-2 aggregation migration PR.

Security & Correctness

1. Bounds Check Ordering in verify_block_signatures

  • File: crates/blockchain/src/store.rs
  • Lines: 1225-1237
  • Issue: The validator index bounds check (block.proposer_index >= num_validators) occurs after the proposer bitfield is validated against block.proposer_index. While the bitfield check ensures the index is the only one set, an out-of-bounds index in a maliciously crafted block would trigger the bounds check only after the proposer lookup logic conceptually begins.
  • Recommendation: Move the bounds check to immediately after extracting proposer_info, before constructing proposer_bits, to fail fast on malformed input.

2. Inconsistent Error Types for Structural Mismatches

  • File: crates/blockchain/src/store.rs
  • Lines: 1215-1225
  • Issue: Slot mismatches return AttestationSignatureMismatch (which confusingly carries length counts), while message root mismatches return ParticipantsMismatch. These should ideally be distinct error variants (e.g., SlotMismatch, MessageMismatch) to aid debugging.
  • Note: This is non-critical but affects observability.

3. Hardcoded Limit in Test Fixtures

  • File: crates/common/test-fixtures/src/verify_signatures.rs
  • Line: 161
  • Issue: The check if attestation_t1s.len() >= 17 uses a magic number instead of the defined constant MAX_ATTESTATIONS_DATA + 1.
  • Recommendation: Import or define MAX_ATTESTATIONS_DATA in this crate to avoid drift if the protocol constant changes.

Code Quality & Maintainability

4. Magic Number Usage

  • File: crates/common/types/src/block.rs
  • Line: 142
  • Issue: The limit 17 for TypeOneInfos is defined here, but used as a literal 17 in the test fixtures mentioned above. Consider exporting a named constant.

5. Clone Efficiency in extend_proofs_greedily

  • File: crates/blockchain/src/store.rs
  • Line: 1053
  • Code: let bits = proof.info.participants.clone();
  • Note: The AggregationBits clone is necessary here since proof is borrowed immutably while selected accumulates owned data. This is acceptable given the bounded size of bitfields.

6. SSZ Stub Documentation

  • File: crates/common/types/src/block.rs
  • Lines: 200-215 (TypeTwoMultiSignature::from_type_1s)
  • Observation: The function correctly leaves proof bytes empty per the Phase-3 stub design. The extensive doc comment explaining this is excellent and critical for security review.

Memory Safety

7. SSZ Decoding Limits

  • File: crates/blockchain/src/store.rs
  • Lines: 517-518, 1210-1211
  • Observation: Proper use of map_err to convert SSZ decoding failures into StoreError variants prevents panics on malicious input. The ByteListMiB type ensures a 1 MiB cap on proof sizes.

8. Bitfield Allocation in for_proposer

  • File: crates/common/types/src/block.rs
  • Lines: 182-183
  • Code: AggregationBits::with_length(proposer_index as usize + 1)
  • Note: While proposer_index is a u64, the validator set size is practically bounded by protocol constants (millions, not billions), so the usize conversion and allocation size are safe. No overflow risk in realistic scenarios.

Testing

9. Test Skipping Strategy

  • File: crates/common/types/tests/ssz_spectests.rs
  • Lines: 68-77
  • Observation: The skipped tests for SignedBlock, BlockSignatures, etc., are correctly flagged with the TODO(type1-type2) marker. This is an acceptable temporary measure for a phased migration, provided the follow-up task is tracked.

10. Helper Function Duplication

  • Observation: make_signed_block_proof is defined in store.rs tests and similarly logic appears in fork_choice.rs fixtures. Consider centralizing this in test-fixtures if it grows further, though current duplication is manageable.

Architectural Approval

The structural verification approach in verify_block_signatures (lines 1165-1207) correctly implements the Phase-3 security model:

  1. Cryptographic verification happens at gossip ingestion (on_gossip_aggregated_attestation)
  2. Block-level verification ensures the merged Type-2 proof's metadata aligns with block contents
  3. The proposer signature is structurally validated as a singleton Type-1 at the end of the list

This is sound because the fork-choice rule only needs to account for votes (participants), not re-verify their cryptography at block import time.

Summary

The PR is correct and well-documented. The Type-1/Type-2 abstraction is cleanly implemented with appropriate SSZ bounds. Minor issues are limited to error variant consistency and magic number usage. The temporary removal of parallel crypto verification is appropriately flagged and safe given the gossip-layer verification remains intact.

Recommended changes before merge:

  • Move the proposer index bounds check earlier in verify_block_signatures (Item 1).
  • Replace hardcoded 17 with the named constant in test fixtures (Item 3).

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

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now I have enough context to write a thorough review. Let me compile my findings.


PR #361refactor(types): port leanSpec Type-1 / Type-2 aggregation envelope

Overview

This PR ports the typed two-level multi-signature envelope from an upstream WIP commit, replacing the flat BlockSignatures + AggregatedSignatureProof wire format with TypeOneMultiSignature / TypeTwoMultiSignature. The migration is done in three well-separated phases (additive types → gossip layer → block wire + storage). The PR description is thorough and the code is production-aware about being a WIP: it's opened as a draft.


Security (Acknowledged Regressions)

The PR knowingly trades two crypto guarantees for structural-only checks. These are documented but worth enumerating for reviewers who may not read the description:

  • Block-level attestation crypto is gone. verify_block_signatures no longer calls verify_aggregated_signature per attestation. It only checks structural alignment (counts, slots, messages, participant indices). The gossip path still verifies individual Type-1 proofs, so the safety net exists, but a block assembled from gossip-verified proofs is not re-verified at block ingestion.
  • Proposer signature is no longer cryptographically verified. The old XMSS is_valid call is gone. The new check only confirms that proposer_info.message == block_root, proposer_info.slot == block.slot, and proposer_bits == [block.proposer_index]. Any peer can produce a structurally valid proposer entry.

Both are documented and tied to a concrete follow-up (lean_multisig real verify_type_2). Tracking via TODO(type1-type2) is appropriate, but these TODOs should have a linked issue or milestone to prevent them from going stale.


Bug: Wrong Error Variants in verify_block_signatures

In crates/blockchain/src/store.rs, the structural check loop uses incorrect error variants:

// Slot mismatch — but returns AttestationSignatureMismatch (count-mismatch semantics)
if info.slot != attestation.data.slot {
    return Err(StoreError::AttestationSignatureMismatch {
        signatures: merged.info.len(),
        attestations: attestations.len(),
    });
}

// Message mismatch — but returns ParticipantsMismatch (bitfield semantics)
if info.message != attestation.data.hash_tree_root() {
    return Err(StoreError::ParticipantsMismatch);
}

AttestationSignatureMismatch carries signatures/attestations count fields that are misleading when the mismatch is a slot value. ParticipantsMismatch for a message hash mismatch is actively confusing — the two conditions are unrelated.

The minimal fix is to add dedicated variants: AttestationSlotMismatch and AttestationMessageMismatch. If new variants are undesirable before the full migration lands, at minimum the error constants should be consistent (ParticipantsMismatch for everything structural would be less wrong than mixing variants).


Misused Error Name: ProposerSignatureDecodingFailed

StoreError::ProposerSignatureDecodingFailed is now returned from both on_block_core and verify_block_signatures when TypeTwoMultiSignature::from_ssz_bytes fails. That's a merged-proof decode failure, not a proposer-signature decode failure. Log-mining and metric labelling will misattribute these failures to the proposer key. A BlockProofDecodingFailed (or at least MergedProofDecodingFailed) variant is warranted.


Duplicate / Magic Constant for the TypeOneInfos Limit

Three different places encode the same value:

  • pub type TypeOneInfos = SszList<TypeOneInfo, 17>types/block.rs
  • const MAX_ATTESTATIONS_DATA: usize = 16;test-fixtures/src/fork_choice.rs (local mirror with a comment explaining why it can't be imported)
  • if attestation_t1s.len() >= 17 {test-fixtures/src/verify_signatures.rs (raw magic number, no comment)

The 17 in verify_signatures.rs is unexplained to a reader and will silently diverge if the limit changes. At minimum add a comment: // MAX_ATTESTATIONS_DATA + 1 = 16 + 1. Better: export the constant from ethlambda-types so it can be shared without a dep-cycle (it belongs in types, not blockchain).


from_type_1s Panics Rather Than Returning an Error

pub fn from_type_1s(type_1s: Vec<TypeOneMultiSignature>) -> Self {
    ...
    let info = TypeOneInfos::try_from(infos)
        .expect("type-1 infos within MAX_ATTESTATIONS_DATA + 1 limit");

The call site in propose_block satisfies this invariant (capped at MAX_ATTESTATIONS_DATA + 1 = 17), but this is an infallible public constructor with a hidden precondition. A caller in tests or future code that passes > 17 items gets a panic rather than a typed error. Either make the function return Result<Self, _> or make it private.


for_proposer: as usize + 1 Unchecked on 32-bit Targets

let mut participants = AggregationBits::with_length(proposer_index as usize + 1)
    .expect("validator index fits");

On 64-bit targets this is fine for any realistic validator index. On 32-bit it wraps if proposer_index >= u32::MAX. The codebase targets Linux x86-64, so this is low priority, but the expect message ("validator index fits") is misleading — the with_length error is about length, not the index value. A more informative message like "AggregationBits allocation for proposer_index {proposer_index}" would help diagnostics.


Missing Structural-Verification Tests

The verify_signatures_rejects_participants_mismatch test is updated, but the new structural check has two additional failure modes with no tests:

  • info.slot != attestation.data.slot (slot mismatch path)
  • info.message != attestation.data.hash_tree_root() (message mismatch path)
  • Proposer-info structural rejection (wrong message, wrong slot, wrong bit count)

These are mechanical to add using the existing make_signed_block_proof helper and should be in before the PR merges.


Dropped Metrics Coverage

The old verify_block_signatures incremented metrics::inc_pq_sig_aggregated_signatures_valid/invalid() per attestation. The new structural loop has no metric increments. Observability for block-level verification is now a single timing log line. The gossip path still tracks per-attestation crypto metrics, so the dashboard won't go dark, but block-level anomalies (structurally malformed blocks) won't surface in Prometheus.


Minor: Inconsistent Field Init Order in SignedAggregatedAttestation Construction

In crates/net/rpc/src/test_driver.rs:

let aggregated = SignedAggregatedAttestation {
    proof: TypeOneMultiSignature::new(...),
    data,
};

Field ordering (proof before data) differs from the struct definition (data first, proof second) and from other construction sites. Rust doesn't care, but it's a minor style inconsistency worth aligning.


What's Done Well

  • The phased migration strategy (additive → gossip → block wire) is sound and each phase compiled independently.
  • The gossip-path crypto (on_gossip_aggregated_attestation) is unchanged and remains the real safety gate.
  • The new for_proposer / empty / participant_indices helpers on TypeOneMultiSignature cover the common construction patterns cleanly.
  • The to_blank_signed_block oversized-block guard is correctly placed: process_block rejects it before storage, so the empty-proof fallback is safe for tests.
  • SSZ round-trip tests for all three new containers (TypeOneInfo, TypeOneMultiSignature, TypeTwoMultiSignature) are present and cover capacity limits.
  • The Table::BlockSignatures rename deferral is the right call — RocksDB CF renames are non-trivial and this is correctly left for a follow-up.
  • Skip markers (TODO(type1-type2)) are scoped, consistent, and have a clear unwinding path.

Summary of Required Changes Before Merge

  1. Fix the two wrong error variants in verify_block_signatures (slot mismatch, message mismatch).
  2. Rename or add the BlockProofDecodingFailed error variant to replace the misused ProposerSignatureDecodingFailed.
  3. Add a comment (or constant) for the raw 17 in test-fixtures/src/verify_signatures.rs.
  4. Add tests for the slot-mismatch and message-mismatch structural-check branches.

Items 3–4 are lower priority but should land before the draft is promoted to ready for review.


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

@github-actions
Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. Block import no longer authenticates either the proposer signature or the block-carried attestation proofs. TypeTwoMultiSignature::from_type_1s drops every child proof and emits an empty merged proof (block.rs:179); block production then serializes that into SignedBlock.proof (lib.rs:341); and verify_block_signatures now only checks metadata alignment / index bounds (store.rs:1207, store.rs:1230). A peer can therefore forge a block with arbitrary attestation participation and a fake proposer signature as long as the info list mirrors the block body. For a consensus client, that is a release blocker because it directly compromises fork choice and justification/finalization.

  2. Block-imported “info-only” proofs can overwrite usable proofs and then poison later aggregation. on_block_core inserts TypeOneMultiSignature { info, proof: ByteListMiB::default() } into the known payload pool (blockchain/store.rs:543). PayloadBuffer::push deduplicates only by participant coverage, so a larger block-derived placeholder can evict a real verified proof (storage/store.rs:143, storage/store.rs:148). Those entries are later reused as aggregation children (aggregation.rs:185), but aggregate_mixed deserializes every child proof (crypto.rs:200), so the empty placeholders can turn late-arriving real signatures into ChildDeserializationFailed instead of extending the aggregate.

  3. Gossip aggregated-attestation ingestion accepts inconsistent TypeOneInfo metadata and stores it verbatim. The proof bytes are verified against aggregated.data (blockchain/store.rs:378), then the unchecked TypeOneMultiSignature is stored (blockchain/store.rs:394). On the common no-duplicate path, compact_attestations returns that proof unchanged (blockchain/store.rs:929), and block verification later rejects mismatched info.slot / info.message (blockchain/store.rs:1236). A malicious peer can use valid proof bytes with poisoned metadata to make a proposer build blocks that fail its own verifier. The fix is to normalize TypeOneInfo from the verified AttestationData before storing.

Notes
Regression coverage is thin while this migration is in flight: SSZ spectests for SignedBlock / SignedAggregatedAttestation are skipped (ssz_spectests.rs:66), and the signature fixture suite explicitly skips test_invalid_proposer_signature (signature_spectests.rs:18).

I couldn’t run cargo test here because rustup tried to create temp files under read-only /home/runner/.rustup.


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

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 12, 2026

Greptile Summary

This PR ports the TypeOneMultiSignature / TypeTwoMultiSignature two-level envelope from the upstream anshalshukla/leanSpec@0ab09dd commit, replacing the flat AggregatedSignatureProof + BlockSignatures pair throughout the gossip pipeline, block builder, and wire format. Block-level verification is intentionally downgraded to a structural-only check (mirroring the upstream stub) while per-attestation crypto verification continues to run at gossip ingestion; the real lean_multisig merging primitive is deferred.

  • Type plumbing (block.rs): TypeOneInfo, TypeOneMultiSignature, TypeTwoMultiSignature, and BytecodeClaim are added; legacy containers removed; SSZ round-trip and capacity unit tests included.
  • Gossip layer (aggregation.rs, storage/store.rs): AggregatedGroupOutput, PayloadBuffer, and all buffer APIs migrate cleanly to TypeOneMultiSignature; per-attestation verify_aggregated_signature call preserved unchanged.
  • Block wire + structural verification (blockchain/store.rs, lib.rs): propose_block folds all Type-1s plus a singleton proposer Type-1 into a merged Type-2 blob; verify_block_signatures is rewritten as a structural alignment check with no crypto; on_block_core re-decodes the same blob a second time to populate the fork-choice payload buffer.

Confidence Score: 3/5

The gossip-layer crypto is intact, but on_block_without_verification now enforces structural proof validity unconditionally, which is a silent behavioral change worth verifying before merging.

The gossip-layer crypto is intact and the type migration across aggregation, storage, and block-production is clean. The unconditional proof decode in on_block_core changes the contract of on_block_without_verification for any caller that passes a default/placeholder proof blob, and the reuse of AttestationSignatureMismatch/ParticipantsMismatch for slot and message mismatches will produce confusing diagnostics when these checks fire against real peers.

crates/blockchain/src/store.rs — both the unconditional proof decode in on_block_core and the reused error variants for slot/message mismatches in verify_block_signatures warrant a second look.

Important Files Changed

Filename Overview
crates/common/types/src/block.rs Introduces TypeOneMultiSignature, TypeTwoMultiSignature, TypeOneInfo, and BytecodeClaim; removes legacy BlockSignatures/AttestationSignatures/AggregatedSignatureProof. SSZ round-trips and capacity limits look correct; from_type_1s stub intentionally leaves proof bytes empty as documented.
crates/blockchain/src/store.rs Rewrites verify_block_signatures as a structural-only check and adds inline proof decode in on_block_core; the merged proof is decoded twice on the verify=true path, and on_block_without_verification now enforces structural proof validity unconditionally — a behavioral change. Wrong error variants for slot/message mismatches.
crates/blockchain/src/aggregation.rs Replaces AggregatedSignatureProof with TypeOneMultiSignature throughout; aggregate_job correctly populates TypeOneInfo with message, slot, and participants. resolve_child_pubkeys and select_proofs_greedily updated cleanly.
crates/blockchain/src/lib.rs Assembles the merged Type-2 proof in propose_block by wrapping the proposer XMSS as a singleton Type-1 and folding all Type-1s via from_type_1s; logic is correct and matches verify_block_signatures expectations.
crates/common/test-fixtures/src/verify_signatures.rs Fixture-to-SignedBlock converters updated to fold attestation Type-1s plus proposer Type-1 into a merged Type-2; both From and try_into_signed_block_with_proofs use zip which silently truncates if signature and attestation counts diverge, though verify_block_signatures would still catch the length mismatch.
crates/storage/src/store.rs Switches PayloadEntry.proofs and all buffer APIs from AggregatedSignatureProof to TypeOneMultiSignature; write_signed_block and get_signed_block now store/load ByteListMiB blobs in the existing BlockSignatures column family (deferred rename). Subsumption logic correctly migrated to info.participants.

Sequence Diagram

sequenceDiagram
    participant Gossip
    participant BlockChain as BlockChainServer
    participant Store as blockchain::store
    participant Storage as storage::store

    Note over Gossip,Storage: Gossip path - per-attestation crypto still runs here
    Gossip->>Store: on_gossip_aggregated_attestation(SignedAggregatedAttestation)
    Store->>Store: verify_aggregated_signature(proof.proof, pubkeys, message, slot)
    Store->>Storage: insert_new_aggregated_payload(TypeOneMultiSignature)

    Note over BlockChain,Storage: Block production
    BlockChain->>Store: produce_block_with_signatures(slot, validator_id)
    Store->>Store: build_block() returns (Block, Vec TypeOneMultiSignature)
    BlockChain->>BlockChain: TypeOneMultiSignature::for_proposer(proposer_index, xmss_sig, block_root, slot)
    BlockChain->>BlockChain: TypeTwoMultiSignature::from_type_1s(all_t1s) - stub, proof bytes empty
    BlockChain->>BlockChain: SSZ-encode to ByteListMiB to SignedBlock.proof

    Note over BlockChain,Storage: Block ingestion
    BlockChain->>Store: on_block(SignedBlock)
    Store->>Store: verify_block_signatures(state, signed_block) - structural only
    Store->>Store: TypeTwoMultiSignature::from_ssz_bytes(proof) - decode 1
    Store->>Store: "check info.len() == attestations.len() + 1"
    Store->>Store: check per-att message, slot, participants
    Store->>Store: "check proposer entry - message==block_root, single bit"
    Store->>Store: TypeTwoMultiSignature::from_ssz_bytes(proof) - decode 2, redundant
    Store->>Store: insert info-only TypeOneMultiSignature entries into known_payloads
    Store->>Storage: insert_signed_block(block_root, SignedBlock)
Loading
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/blockchain/src/store.rs:526-534
**Redundant SSZ decode of `TypeTwoMultiSignature`**

When `verify = true`, `verify_block_signatures` already decodes and validates the merged proof (line 1216), performing the same `from_ssz_bytes` call and the same `info.len() == attestations.len() + 1` check. This block then repeats both operations unconditionally. On the hot block-ingestion path this means every block with signature verification pays for two SSZ decodes of potentially large proof blobs. Consider passing the already-decoded `TypeTwoMultiSignature` out of `verify_block_signatures` (or into `on_block_core`) rather than re-decoding from the raw bytes.

### Issue 2 of 3
crates/blockchain/src/store.rs:1236-1244
**Wrong error variants for non-count mismatches**

Two distinct failure modes are reported with misleading error types. When `info.slot != attestation.data.slot` the code returns `AttestationSignatureMismatch { signatures, attestations }` with the *same* counts — there is no count mismatch, so callers parsing the error numbers will be confused. When `info.message != attestation.data.hash_tree_root()` the code returns `ParticipantsMismatch`, but the participants may be identical — it is the signed message that diverges. These errors surface in logs and in the Hive test-driver's `verify_signatures/run` response, making triage harder when these checks start firing against real peers.

### Issue 3 of 3
crates/blockchain/src/store.rs:526-527
**`on_block_without_verification` now rejects structurally malformed proofs**

Previously, `on_block_without_verification` would happily zip over `attestation_signatures` regardless of their correctness. Now lines 526–534 run unconditionally (not guarded by `if verify`), so a call with a proof blob that fails SSZ decode *or* whose `info.len()` doesn't match attestation count will return an error even on the "no verification" path. This is a silent behavioral change for callers (tests, replay tools, benchmark harnesses) that construct `SignedBlock` with a minimal or placeholder proof. The intent of `on_block_without_verification` was to bypass signature checks entirely; it now enforces structural proof validity as a side effect of the fork-choice vote insertion logic.

Reviews (1): Last reviewed commit: "chore: remove comment" | Re-trigger Greptile

Comment on lines +526 to +534
let merged = TypeTwoMultiSignature::from_ssz_bytes(signed_block.proof.iter().as_slice())
.map_err(|_| StoreError::ProposerSignatureDecodingFailed)?;
let expected_info_len = aggregated_attestations.len() + 1;
if merged.info.len() != expected_info_len {
return Err(StoreError::AttestationSignatureMismatch {
signatures: merged.info.len(),
attestations: aggregated_attestations.len(),
});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant SSZ decode of TypeTwoMultiSignature

When verify = true, verify_block_signatures already decodes and validates the merged proof (line 1216), performing the same from_ssz_bytes call and the same info.len() == attestations.len() + 1 check. This block then repeats both operations unconditionally. On the hot block-ingestion path this means every block with signature verification pays for two SSZ decodes of potentially large proof blobs. Consider passing the already-decoded TypeTwoMultiSignature out of verify_block_signatures (or into on_block_core) rather than re-decoding from the raw bytes.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 526-534

Comment:
**Redundant SSZ decode of `TypeTwoMultiSignature`**

When `verify = true`, `verify_block_signatures` already decodes and validates the merged proof (line 1216), performing the same `from_ssz_bytes` call and the same `info.len() == attestations.len() + 1` check. This block then repeats both operations unconditionally. On the hot block-ingestion path this means every block with signature verification pays for two SSZ decodes of potentially large proof blobs. Consider passing the already-decoded `TypeTwoMultiSignature` out of `verify_block_signatures` (or into `on_block_core`) rather than re-decoding from the raw bytes.

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

Comment on lines +1236 to +1244
if info.slot != attestation.data.slot {
return Err(StoreError::AttestationSignatureMismatch {
signatures: merged.info.len(),
attestations: attestations.len(),
});
}
if info.message != attestation.data.hash_tree_root() {
return Err(StoreError::ParticipantsMismatch);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Wrong error variants for non-count mismatches

Two distinct failure modes are reported with misleading error types. When info.slot != attestation.data.slot the code returns AttestationSignatureMismatch { signatures, attestations } with the same counts — there is no count mismatch, so callers parsing the error numbers will be confused. When info.message != attestation.data.hash_tree_root() the code returns ParticipantsMismatch, but the participants may be identical — it is the signed message that diverges. These errors surface in logs and in the Hive test-driver's verify_signatures/run response, making triage harder when these checks start firing against real peers.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1236-1244

Comment:
**Wrong error variants for non-count mismatches**

Two distinct failure modes are reported with misleading error types. When `info.slot != attestation.data.slot` the code returns `AttestationSignatureMismatch { signatures, attestations }` with the *same* counts — there is no count mismatch, so callers parsing the error numbers will be confused. When `info.message != attestation.data.hash_tree_root()` the code returns `ParticipantsMismatch`, but the participants may be identical — it is the signed message that diverges. These errors surface in logs and in the Hive test-driver's `verify_signatures/run` response, making triage harder when these checks start firing against real peers.

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

Comment on lines +526 to +527
let merged = TypeTwoMultiSignature::from_ssz_bytes(signed_block.proof.iter().as_slice())
.map_err(|_| StoreError::ProposerSignatureDecodingFailed)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 on_block_without_verification now rejects structurally malformed proofs

Previously, on_block_without_verification would happily zip over attestation_signatures regardless of their correctness. Now lines 526–534 run unconditionally (not guarded by if verify), so a call with a proof blob that fails SSZ decode or whose info.len() doesn't match attestation count will return an error even on the "no verification" path. This is a silent behavioral change for callers (tests, replay tools, benchmark harnesses) that construct SignedBlock with a minimal or placeholder proof. The intent of on_block_without_verification was to bypass signature checks entirely; it now enforces structural proof validity as a side effect of the fork-choice vote insertion logic.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 526-527

Comment:
**`on_block_without_verification` now rejects structurally malformed proofs**

Previously, `on_block_without_verification` would happily zip over `attestation_signatures` regardless of their correctness. Now lines 526–534 run unconditionally (not guarded by `if verify`), so a call with a proof blob that fails SSZ decode *or* whose `info.len()` doesn't match attestation count will return an error even on the "no verification" path. This is a silent behavioral change for callers (tests, replay tools, benchmark harnesses) that construct `SignedBlock` with a minimal or placeholder proof. The intent of `on_block_without_verification` was to bypass signature checks entirely; it now enforces structural proof validity as a side effect of the fork-choice vote insertion logic.

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

  Move the per-block attestation-data cap to ethlambda_types::block as the
  canonical home, used by the wire types, the blockchain layer, and the
  test-fixtures crate. Replaces the duplicated literal in
  ethlambda-blockchain and the mirrored constant in
  ethlambda-test-fixtures, and derives the TypeOneInfos SSZ-list limit
  from MAX_ATTESTATIONS_DATA + 1 so the proposer-plus-attestations bound
  stays in one place.

  Addresses MegaRedHand review comment on PR #361.
Copy link
Copy Markdown
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread crates/common/types/src/block.rs Outdated
@MegaRedHand MegaRedHand merged commit 273145b into devnet5 May 12, 2026
3 checks passed
@MegaRedHand MegaRedHand deleted the type1-type2-aggregation branch May 12, 2026 19:25
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