refactor: use libssz instead of ethereum_ssz

[MegaRedHand]

No description provided.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR migrates ethlambda from the legacy ethereum_ssz/tree_hash ecosystem to a new in-house SSZ implementation (libssz). The change is large but necessary to eliminate external dependencies and align with the project's minimalist philosophy.

✅ Correctness & Consensus Safety

• State transition logic preserved: All hash-tree-root computations, bitfield operations, and serialization paths have been mechanically updated without altering consensus semantics.
• Genesis & checkpoint sync: H256 construction from hash_tree_root() is now explicit (H256(state.hash_tree_root())), preventing silent type mismatches.

⚠️ Security & Edge Cases

1. Bitfield capacity checks (crates/blockchain/src/store.rs:969, state_transition/src/justified_slots_ops.rs:55)

   • JustifiedSlots::with_length(required_capacity) can panic on overflow. Add explicit bounds checking before allocation.

2. Signature parsing (crates/common/types/src/signature.rs:40)

   • SignatureParseError wraps format!("{e:?}"), leaking internal details. Use Display instead of Debug for production logs.

3. SSZ list truncation (crates/storage/src/store.rs:119)

   • ErrorMessage::try_from(truncated) silently truncates long strings. Assert or log when truncation occurs to avoid silent data loss.

🔍 Performance & Idiomatic Rust

• Avoid repeated bit counting (crates/blockchain/src/store.rs:505-510)
  Replace the manual byte-wise count with aggregated.proof.participants.count_ones() if SszBitlist exposes it.

• Redundant clones (crates/common/types/src/genesis.rs:56)
  pubkey.clone() for a 52-byte array is unnecessary; use *pubkey.

• Iterator allocation (state_transition/src/lib.rs:248-254)
  The collect::<Vec<bool>>() can be avoided by iterating directly over the bitfield range.

📦 Dependency Hygiene

• Duplicate ethereum_ssz in crypto (crates/common/crypto/Cargo.toml)
  The legacy ethereum_ssz is only needed for Devnet2XmssAggregateSignature. Gate it behind a #[cfg(feature = "legacy-ssz")] to prevent accidental usage elsewhere.

• Unused imports (crates/net/p2p/src/req_resp/codec.rs:17)
  Remove the unused SszDecode import.

✅ Testing & Maintainability

• Test data regeneration (crates/net/p2p/src/gossipsub/encoding.rs:59)
  The ignored test references old field order. Regenerate test vectors to match the new SszEncode layout.

• Bitfield iteration (crates/common/types/src/attestation.rs:71-77)
  The new iterator is correct but less efficient. Consider adding a set_bits() method to SszBitlist for direct iteration.

Action Items

1. Add bounds checks for bitfield allocations.
2. Replace Debug with Display in error messages.
3. Remove legacy ethereum_ssz from non-crypto crates.
4. Regenerate test fixtures for P2P encoding tests.

Verdict: The migration is sound with minor polish needed. No consensus-breaking issues detected.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

1. High - test pre-state is no longer faithfully loaded from fixtures
   In crates/common/test-fixtures/src/lib.rs:158 and crates/common/test-fixtures/src/lib.rs:161, justified_slots and justifications_validators are hard-reset to empty instead of being reconstructed from fixture data (TestState still carries both at lines 127-133).
   This can invalidate consensus/state-transition spectests by mutating the initial state, masking real regressions (especially justification/finalization behavior).
   Suggested fix: reconstruct both fields from fixture contents (with finalized-slot-relative mapping for justified_slots if representation changed).

2. Medium - wire-format interoperability risk is currently unprotected
   BlockSignatures SSZ field order is now attestation_signatures then proposer_signature in crates/common/types/src/block.rs:51, and the cross-client decode test is explicitly ignored in crates/net/p2p/src/gossipsub/encoding.rs:59.
   Given SSZ container ordering is part of the hash/wire format, this is a potential network-compatibility break unless all peers are upgraded in lockstep. At minimum, keep an active compatibility test for both formats during migration.

3. Low - participant count metric may be inaccurate
   In crates/blockchain/src/store.rs:505, counting participants via participants.as_bytes().count_ones() depends on internal bitlist byte representation. This can drift from logical participant count if trailing/non-logical bits ever appear.
   Safer/clearer: aggregated.proof.participant_indices().count().

Overall, the SSZ migration itself looks mostly consistent (hash root and encode/decode replacements are generally coherent), but Point 1 is a significant correctness gap in consensus test coverage.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have enough context. Let me write up the review.

---

PR #242 Review: refactor: use libssz instead of ethereum_ssz

This is a large mechanical migration from ethereum_ssz/ssz_types/tree_hash to the in-house libssz (lambdaclass/libssz). The API renaming is consistent and systematic (Encode→SszEncode, as_ssz_bytes()→to_ssz(), TreeHash→HashTreeRoot, tree_hash_root()→hash_tree_root(), etc.). The H256 type is replaced with a bespoke newtype. Overall the mechanical parts look clean, but there are a few issues worth addressing.

---

Bug: H256 serializes without 0x prefix

crates/common/types/src/primitives.rs, H256 Serialize impl

impl serde::Serialize for H256 {
    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        serializer.serialize_str(&format!("{:x}", self))
    }
}

format!("{:x}", self) invokes LowerHex, which emits hex digits with no prefix (e.g. "aabbcc..."). The old ethereum-types::H256 serialized as "0x...". Meanwhile Display correctly adds "0x", but isn't used here.

The deserializer accepts both forms, so round-tripping works internally. But any external consumer — Ream, Zeam, qlean, Lantern, or standard tooling — receiving JSON from the RPC endpoints will get block roots and state roots without the 0x prefix, breaking interoperability. Checkpoint and BlockHeader both derive Serialize and embed H256, so all JSON API responses are affected.

Fix: use format!("{}", self) (the Display impl) instead of format!("{:x}", self).

---

Bug: H256::from_slice silently pads/truncates

crates/common/types/src/primitives.rs

pub fn from_slice(bytes: &[u8]) -> Self {
    let mut arr = [0u8; 32];
    let len = bytes.len().min(32);
    arr[..len].copy_from_slice(&bytes[..len]);
    Self(arr)
}

If fewer than 32 bytes are supplied the result is silently zero-padded, producing a different hash from the one intended. This is particularly dangerous for hash/root values — a caller passing a truncated slice would get an invalid (but non-panicking) H256 that silently participates in fork choice or state root comparisons. This function should either assert/panic on wrong-length input or return Result<Self, _>.

---

Off-by-one in num_participants (logging only, but misleading)

crates/blockchain/src/store.rs

let num_participants: u32 = aggregated
    .proof
    .participants
    .as_bytes()
    .iter()
    .map(|b| b.count_ones())
    .sum();

SSZ bitlists are encoded with a sentinel 1 bit appended after the last data bit, so count_ones() across raw bytes over-counts by 1. For example, a 3-element bitlist [1, 0, 1] encodes as 0b00001101, giving count_ones() = 3 rather than 2.

This is used only for the info! log line, so it has no consensus impact, but it will consistently misreport participant counts in monitoring/debugging. The old num_set_bits() presumably accounted for the sentinel. Consider restoring or using a method from libssz that operates on the logical length.

---

Missing Deserialize on ValidatorPubkeyBytes

crates/common/types/src/state.rs

ValidatorPubkeyBytes implements Serialize but not Deserialize. The old pub type ValidatorPubkeyBytes = [u8; 52] got blanket impls for both. If any code path deserializes a Validator or ValidatorPubkeyBytes from JSON/YAML (e.g. checkpoint sync response parsing, test fixture loading), it will fail to compile or panic at runtime. Verify that all deserialization sites either go through SSZ (which is covered) or have an explicit deserialization path.

---

set() panics silently discarded (minor)

crates/blockchain/src/store.rs, crates/blockchain/state_transition/src/justified_slots_ops.rs

The old bits.set(vid, true).expect("...") made it visible when an out-of-bounds set was attempted. The new bits.set(vid as usize, true) has no returned Result, so the behaviour (panic vs. no-op) on an out-of-bounds index depends entirely on how libssz implements set. The logic that computes capacity (max_id + 1) before calling set looks correct, but this is now an unverified invariant. A comment or a debug assertion (debug_assert!(vid < bits.len())) would preserve the earlier intent.

---

Error formatting style inconsistency (minor)

crates/blockchain/src/key_manager.rs and crates/common/types/src/signature.rs

.map_err(|e| KeyManagerError::SignatureConversionError(format!("{e:?}")))?;

{e:?} (Debug) is used instead of {e} (Display). The existing codebase uses %err (Display) in tracing and consistent Display formatting in error messages. Using Debug adds noise ("..." wrappers, Rust enum path prefixes) to user-facing error strings. Change to format!("{e}").

---

Positive notes

• The extend_to_slot rewrite (replacing union with explicit bit copy) is logically correct and the early-exit guard ensures it only extends, never shrinks.
• The pinning of libssz to a specific git commit (a6ffa33) is good practice for reproducibility.
• The ValidatorPubkeyBytes newtype with manual SszEncode/SszDecode/HashTreeRoot impls is a clean solution to the 52-byte-array limitation.
• The SIGNATURE_SIZE: usize = 3112 constant replacing the typenum arithmetic (Diff<U3600, U488>) is much more readable.
• Retaining ethereum_ssz in ethlambda-crypto with a clear comment explaining why (external Devnet2XmssAggregateSignature uses the old API) is the right pragmatic approach.
• The shift_window rewrite looks correct.
• All uses of H256(x.hash_tree_root()) are consistent and mechanical.

---

Automated review by Claude (Anthropic) · custom prompt

[pablodeymo]

lean-multisig is still using ethereum_ssz::Encode/Decode traits. So we are depending on both libraries

[pablodeymo]

Nice work on the migration, clean and well-structured. CI is green and all tests pass. A few observations:

H256(x.hash_tree_root()) wrapping pattern

There are ~25 call sites wrapping hash_tree_root() in H256(...). I checked libssz and confirmed there's no ethereum_types feature flag — HashTreeRoot returns [u8; 32] by design, which makes sense for a generic no_std library.

Since H256 is now a local type, we can add a convenience trait on our side to reduce the boilerplate. Something like:

// In ethlambda_types::primitives
use ssz_merkle::HashTreeRoot;

pub trait HashTreeRootExt: HashTreeRoot {
    fn tree_hash_root(&self) -> H256 {
        H256(self.hash_tree_root())
    }
}
impl<T: HashTreeRoot> HashTreeRootExt for T {}

This would turn H256(block.hash_tree_root()) back into block.tree_hash_root() everywhere. Not a blocker for this PR — could be a quick follow-up.

ethereum_ssz residual in crypto crate

The comment in crates/common/crypto/Cargo.toml explains this well — lean-multisig still uses the old ethereum_ssz traits. Just flagging it for tracking so it gets cleaned up when lean-multisig migrates.

ValidatorPubkeyBytes manual impls

The ~40 lines of manual SszEncode/SszDecode/HashTreeRoot for the 52-byte pubkey newtype are correct but boilerplate. Could libssz support arbitrary [u8; N] via const generics? Would eliminate the need for manual impls on uncommon byte array sizes. Low priority.

Bitlist set() no longer returns Result

The old set(i, true).unwrap() pattern is now just set(i, true). Cleaner, but what happens on out-of-bounds? Silent no-op or panic? Worth documenting the contract.

Overall this is a solid migration. The const generics over typenum are a clear readability win, and removing 6 external deps is great for the dependency tree.