fix: drop HashTreeRoot from XMSS-containing SSZ types

[MegaRedHand]

Summary

• Removes HashTreeRoot derive from SignedAttestation, SignedBlock, and BlockSignatures — the three types that embed XmssSignature.
• Routes those types through a new run_serialization_only_test helper in the SSZ spec-test harness. Encoding and round-trip are still enforced; the hash-tree-root check is skipped for them.

Why

leanSpec's Signature container is SSZ-serialized as fixed bytes (via is_fixed_size=True, for cross-client wire compatibility with Ream) but its hash tree root is computed as a Container (merkleized fields). Our Rust XmssSignature is SszVector<u8, SIGNATURE_SIZE>, whose hash tree root uses merkleize(pack_bytes(...)) — identical serialization, different root.

Since SignedAttestation, SignedBlock, and BlockSignatures are never hashed directly in production code (only their inner Block / AttestationData are), dropping the HashTreeRoot derive is cheaper than mirroring leanSpec's container layout. If any of them gets hashed later, we'll re-derive and match.

Fixes 4 failing test_consensus_containers SSZ tests after the leanSpec bump to e9ddede:

• test_block_signatures_empty
• test_block_signatures_with_attestation
• test_signed_attestation_minimal
• test_signed_block_minimal

Test plan

• cargo test -p ethlambda-types --release --test ssz_spectests passes (112/112)
• CI is green

Follow-ups

• Two stacked PRs follow this one: # adapts the fork-choice harness to the new fixture format, # exercises real signature verification end-to-end.

[github-actions]

🤖 Kimi Code Review

The PR correctly addresses an SSZ spec divergence where XmssSignature serialization (fixed-size bytes) differs from its Merkleization (container). Removing HashTreeRoot from wrapper types (SignedBlock, BlockSignatures, SignedAttestation) prevents accidental use of incorrect roots in consensus-critical paths, leveraging the type system as a safety mechanism.

Key observations:

1. Safety (crates/common/types/src/{block,attestation}.rs): The comments clearly document why HashTreeRoot is omitted. This is critical—if code attempted to hash these wrapper types (e.g., for signing or state transitions), compilation would fail, preventing a consensus bug. Ensure the claim "consumers always hash the inner Block" holds across the codebase (it appears to, as signatures verify against HashTreeRoot(block.message) not HashTreeRoot(block)).

2. Test refactoring (crates/common/types/tests/ssz_spectests.rs): The separation into run_serialization_only_test vs run_typed_test is clean. The new check_ssz_roundtrip helper correctly isolates encoding verification.

   • Minor: The debug_assert_eq! at line 113 (re-encoding domain_value) is redundant since check_ssz_roundtrip already verified this, but harmless as it's debug-only.
   • Minor optimization: check_ssz_roundtrip could return (Vec<u8>, D) to avoid double-deserialization in run_typed_test, but this is negligible for test I/O.

3. Consensus correctness: Acknowledging that wire format (SSZ bytes) must match across clients while Merkle roots of these specific envelopes intentionally diverge is the correct approach. The inner types (Block, AttestationData) retain HashTreeRoot, ensuring state transitions and fork-choice operate on correct roots.

Verdict: Approved. The changes are precise, well-documented, and improve safety by making the intentional spec divergence explicit and compiler-enforced.

---

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

[github-actions]

🤖 Claude Code Review

Now I have a complete picture. Here's my review:

---

PR 302 — fix: drop HashTreeRoot from XMSS-containing SSZ types

Overview

Removes the HashTreeRoot derive macro from SignedAttestation, SignedBlock, and BlockSignatures — the three types whose XmssSignature field is serialized as a fixed-size byte vector but Merkleized as a container by the spec. Splits the SSZ spec test harness into check_ssz_roundtrip (encoding + decode/re-encode) and run_typed_test / run_serialization_only_test, so wire-format conformance is still enforced for these types without performing the divergent hash check.

---

Correctness

The core decision is sound. A search of all hash_tree_root() call sites confirms that SignedBlock, BlockSignatures, and SignedAttestation are never hashed directly in production code:

• store.rs:733 hashes block (inner Block)
• lib.rs:323 hashes signed_block.message (inner Block)
• store.rs:1238, 1497, 1757 hash data / att_data (AttestationData)

Dropping the derive is safe for today's code.

---

Issues

debug_assert_eq! is a no-op under --release

ssz_spectests.rs (around the newly introduced check in run_typed_test):

// Re-encode for the hash computation; cheap relative to the fixture I/O.
debug_assert_eq!(<D as libssz::SszEncode>::to_ssz(&domain_value), expected_bytes);

The test plan runs cargo test ... --release, which strips all debug_assert! / debug_assert_eq! calls. This check is a silent no-op in the CI run that matters. It should either be assert_eq! (if the invariant is worth enforcing) or removed entirely (if the point is just to document intent via a comment).

Double deserialization in run_typed_test

check_ssz_roundtrip deserializes and encodes the fixture internally. run_typed_test then deserializes the same JSON a second time to obtain domain_value for the hash root check:

let expected_bytes = check_ssz_roundtrip::<F, D>(test)?;   // deserialize #1 inside here
// ...
let fixture_value: F = serde_json::from_value(test.value.clone()); // deserialize #2
let domain_value: D = fixture_value.into();

This is test-only code so correctness isn't at risk, but it's worth noting. The cleanest fix would be to return (Vec<u8>, D) from check_ssz_roundtrip, avoiding the redundant work and the dead debug_assert_eq!.

---

Minor Observations

• The inline comments added to attestation.rs and block.rs explaining why HashTreeRoot was omitted are exactly right. Future readers (and the follow-up PRs) will benefit from them.
• run_serialization_only_test as a thin check_ssz_roundtrip(...).map(|_| ()) wrapper is clean and communicates intent clearly.
• The refactoring direction (extracting check_ssz_roundtrip) is good; only the debug_assert_eq! residue undermines it.

---

Summary

The change is correct and well-motivated. The one actionable fix before merge:

• Replace the debug_assert_eq! in run_typed_test with assert_eq! (or remove it), since the tests run in --release and the check is currently invisible to CI.

---

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

[github-actions]

🤖 Codex Code Review

No findings.

This PR looks like the right hardening move: it removes potentially incorrect HashTreeRoot impls from the signed wrapper types instead of leaving silently-wrong roots available at runtime in attestation.rs#L37 and block.rs#L14. I checked the current consensus paths and they hash only the inner consensus objects, not the wrappers: attestation verification uses AttestationData roots in store.rs#L547, aggregated signature verification uses attestation.data.hash_tree_root() in store.rs#L1497, and block handling/gossip uses signed_block.message.hash_tree_root() in lib.rs#L233 and handler.rs#L45.

The SSZ test change in ssz_spectests.rs#L58 is also consistent with that design: serialization compatibility is still enforced for these wire types, while the incorrect tree-hash expectation is no longer treated as passing consensus behavior. The only caveat is future-facing: if the project ever needs SignedBlock/SignedAttestation roots, it should add an explicit spec-correct custom HashTreeRoot impl rather than re-derive it.

I couldn’t run cargo test in this sandbox because Cargo needs writable/networked registry and git cache access.

---

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

[greptile-apps]

Greptile Summary

Drops HashTreeRoot from SignedAttestation, SignedBlock, and BlockSignatures — the three types that embed XmssSignature — because leanSpec Merkleizes the signature as a container while the Rust implementation treats it as a fixed-size byte vector, causing unavoidable root divergence. The SSZ spec-test harness is refactored to extract a shared check_ssz_roundtrip helper and route XMSS-containing types through a new run_serialization_only_test path that enforces encoding/round-trip correctness without checking the hash tree root.

Confidence Score: 5/5

Safe to merge — the only finding is a P2 style suggestion about a debug_assert_eq! that is unreachable in release-mode test runs.

All production-code changes are compile-time only (removed derive macros); any caller of .hash_tree_root() on these types would have caused a compile error, making the change safe by construction. The test refactoring is correct and well-documented. The single P2 finding does not affect correctness.

crates/common/types/tests/ssz_spectests.rs — minor double-deserialization and unreachable debug assertion in run_typed_test.

Important Files Changed

Filename	Overview
crates/common/types/src/attestation.rs	Removes HashTreeRoot derive from SignedAttestation with a clear doc comment explaining the serialization vs. Merkleization mismatch for XmssSignature; all remaining types that do need hashing retain the derive.
crates/common/types/src/block.rs	Removes HashTreeRoot derive from SignedBlock and BlockSignatures with well-documented rationale; Block, BlockHeader, BlockBody, and AggregatedSignatureProof correctly retain the derive.
crates/common/types/tests/ssz_spectests.rs	Extracts shared check_ssz_roundtrip helper and adds run_serialization_only_test for XMSS-containing types; run_typed_test deserializes the fixture twice (once inside the helper, once for the hash-root check) and the debug_assert_eq! guarding that duplication is unreachable in release-mode runs.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[run_ssz_test] --> B{type_name?}
    B -- "Block, State, Attestation,\nBlockHeader, etc." --> C[run_typed_test]
    B -- "SignedAttestation\nSignedBlock\nBlockSignatures" --> D[run_serialization_only_test]

    C --> E[check_ssz_roundtrip\nencode + decode round-trip]
    D --> E

    E --> F{Encoding\nmatch?}
    F -- No --> G[Error: encoding mismatch]
    F -- Yes --> H{Round-trip\nmatch?}
    H -- No --> I[Error: round-trip mismatch]
    H -- Yes --> J[Return expected_bytes]

    C --> K[Deserialize F→D again\ncompute hash_tree_root]
    K --> L{Root\nmatch?}
    L -- No --> M[Error: root mismatch]
    L -- Yes --> N[✅ Pass]

    D --> O[✅ Pass — root skipped\nXmssSignature diverges]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/common/types/tests/ssz_spectests.rs
Line: 111

Comment:
**`debug_assert_eq!` is unreachable in release-mode test runs**

Per the repo's test workflow (`cargo test --release`), `debug_assert_eq!` compiles away in every run, so the consistency guard it intends to provide — that the second `F → D` deserialization produces the same encoding as the one already validated inside `check_ssz_roundtrip` — is never actually exercised.

If the check is meaningful, use `assert_eq!` instead. Alternatively, return the `domain_value` from `check_ssz_roundtrip` alongside `expected_bytes` to eliminate the duplicate deserialization pass entirely and make the guard unnecessary.

```suggestion
    assert_eq!(
        <D as libssz::SszEncode>::to_ssz(&domain_value),
        expected_bytes,
        "re-encoded domain value must match expected bytes"
    );
```

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

_{Reviews (1): Last reviewed commit: "fix: drop HashTreeRoot from XMSS-contain..." | Re-trigger Greptile}

[greptile-apps]

debug_assert_eq! is unreachable in release-mode test runs

Per the repo's test workflow (cargo test --release), debug_assert_eq! compiles away in every run, so the consistency guard it intends to provide — that the second F → D deserialization produces the same encoding as the one already validated inside check_ssz_roundtrip — is never actually exercised.

If the check is meaningful, use assert_eq! instead. Alternatively, return the domain_value from check_ssz_roundtrip alongside expected_bytes to eliminate the duplicate deserialization pass entirely and make the guard unnecessary.

Suggested change

	debug_assert_eq!(<D as libssz::SszEncode>::to_ssz(&domain_value), expected_bytes);
	assert_eq!(
	<D as libssz::SszEncode>::to_ssz(&domain_value),
	expected_bytes,
	"re-encoded domain value must match expected bytes"
	);

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/common/types/tests/ssz_spectests.rs
Line: 111

Comment:
**`debug_assert_eq!` is unreachable in release-mode test runs**

Per the repo's test workflow (`cargo test --release`), `debug_assert_eq!` compiles away in every run, so the consistency guard it intends to provide — that the second `F → D` deserialization produces the same encoding as the one already validated inside `check_ssz_roundtrip` — is never actually exercised.

If the check is meaningful, use `assert_eq!` instead. Alternatively, return the `domain_value` from `check_ssz_roundtrip` alongside `expected_bytes` to eliminate the duplicate deserialization pass entirely and make the guard unnecessary.

```suggestion
    assert_eq!(
        <D as libssz::SszEncode>::to_ssz(&domain_value),
        expected_bytes,
        "re-encoded domain value must match expected bytes"
    );
```

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