Skip to content

feat: add SSZ encoding/decoding support for parsigex#322

Open
varex83agent wants to merge 2 commits intobohdan/dkg-parsigexfrom
bohdan/dkg-ssz
Open

feat: add SSZ encoding/decoding support for parsigex#322
varex83agent wants to merge 2 commits intobohdan/dkg-parsigexfrom
bohdan/dkg-ssz

Conversation

@varex83agent
Copy link
Copy Markdown
Collaborator

Summary

  • Adds low-level SSZ encode/decode helpers (crates/ssz/src/encode.rs, crates/ssz/src/decode.rs)
  • Extends crates/ssz/src/types.rs with SSZ type implementations for eth2api spec types
  • Derives SSZ traits on crates/eth2api/src/spec/* types (phase0, altair, bellatrix, capella, deneb, electra, fulu)
  • Adds crates/core/src/ssz_codec.rs — SSZ codec integration for parsigex signed data
  • Extends parsigex_codec.rs with SSZ-based serialisation alongside existing JSON path
  • Adds SSZ test fixtures under crates/core/testdata/ssz/ generated by test-infra/sszfixtures/
  • Fixes yanked fastrand 2.4.02.4.1 and ignores pre-existing rand 0.8.5 advisory (RUSTSEC-2026-0097) present on base branch

Test plan

  • cargo test --workspace --all-features passes
  • cargo clippy --workspace --all-targets --all-features -- -D warnings passes
  • SSZ round-trip tests in crates/core exercise the new fixtures
  • cargo deny check passes

🤖 Generated with Claude Code

varex83agent and others added 2 commits April 13, 2026 16:11
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@iamquang95
Copy link
Copy Markdown
Collaborator

@claude

@claude
Copy link
Copy Markdown

claude bot commented Apr 14, 2026

Claude finished @iamquang95's task in 4m 52s —— View job


Code Review — feat: add SSZ encoding/decoding support for parsigex

  • Gather context and read repo guidelines
  • Review crates/ssz/src/encode.rs and crates/ssz/src/decode.rs
  • Review crates/ssz/src/types.rs and crates/ssz/src/lib.rs
  • Review crates/core/src/ssz_codec.rs
  • Review crates/core/src/parsigex_codec.rs changes
  • Review crates/eth2api/src/spec/* type changes
  • Review test fixtures and fixture generator
  • Review dependency changes (Cargo.toml, deny.toml)
  • Compile findings and post review

Summary

The PR adds SSZ binary encode/decode support for Charon's parsigex signed-data types. The architecture is clean and the Go-fixture compatibility tests are a good signal for wire-format correctness. However, there are several issues ranging from a potential protocol-level asymmetry to missing bounds checks in the BitList decoder.


Findings


[High] SyncContributionAndProof serialize path has no matching deserialize path

serialize_signed_data handles SyncContributionAndProof (unsigned ContributionAndProof) via encode_contribution_and_proof, but deserialize_signed_data for DutyType::SyncContribution only decodes SignedContributionAndProof (with a 96-byte signature field). Decoding unsigned bytes as a signed struct will fail with an SSZ error, and the function returns UnsupportedDutyType.

If a peer ever serializes a SyncContributionAndProof via serialize_signed_data, the receiving peer cannot deserialize it. The test for this path (marshal_unmarshal_ssz_sync_contribution_and_proof) explicitly bypasses deserialize_signed_data and calls the test-only decode_contribution_and_proof directly — a red flag.

Impact: The serialize path for SyncContributionAndProof is either dead code or a silent protocol bug.

Evidence: crates/core/src/parsigex_codec.rs:107-113 (encode), crates/core/src/parsigex_codec.rs:248-256 (decode), crates/core/src/parsigex_codec.rs:395-415 (test)

Recommendation: Either remove the SyncContributionAndProof branch from serialize_signed_data (if it is truly never transmitted via parsigex), or add a matching deserialize branch for it. Add a round-trip test that goes through deserialize_signed_data. Fix this →


[High] BitList::Decode does not enforce MAX bound

Decode for BitList<MAX> at types.rs:377-385 calls BitList::from_ssz_bytes(bytes.to_vec()) without checking that the decoded bit count is ≤ MAX. By contrast, SszList::Decode at types.rs:103-113 returns an error when values.len() > MAX.

Impact: Malformed or attacker-controlled SSZ input can produce a BitList<MAX> with more bits than the type allows, silently violating the type invariant.

Evidence: crates/ssz/src/types.rs:377-385 (missing check); compare to crates/ssz/src/types.rs:103-113 (SszList does check)

Recommendation: After decoding, add:

if MAX > 0 && value.len() > MAX {
    return Err(DecodeError::BytesInvalid(format!(
        "bitlist length {} exceeds max {MAX}", value.len()
    )));
}

Fix this →


[High] BitList::from_ssz_bytes silently accepts an invalid zero last byte

In types.rs:295-320, when last_byte == 0 the function returns Self::default() (empty) instead of an error. Per the SSZ spec, a valid BitList encoding always has a non-zero last byte (the sentinel bit). Silently returning an empty bitlist for invalid input hides malformed data.

Additionally, the method signature is from_ssz_bytes(ssz: Vec<u8>) -> Self (infallible) while the Decode trait needs Result. This prevents the Decode impl from ever surfacing parse errors.

Evidence: crates/ssz/src/types.rs:295-320

Recommendation: Change from_ssz_bytes to return Result<Self, …> (or create a separate fallible internal helper), so the Decode impl can propagate errors. Fix this →


[Medium] encode_version always reports UnknownVersion(0) on failure

ssz_codec.rs:150-155: when DataVersion::Unknown.to_legacy_u64() fails, the error is SszCodecError::UnknownVersion(0). Version 0 is the legitimate Phase0 encoding, so the error message will read "ssz unknown version: 0" — pointing at Phase0 rather than the actual invalid version.

Evidence: crates/core/src/ssz_codec.rs:150-155

Recommendation: Either store the DataVersion debug name in the error, or use a distinct sentinel like u64::MAX. Fix this →


[Medium] decode_contribution_and_proof is #[cfg(test)] while encode_contribution_and_proof is pub

ssz_codec.rs:125-130 gates the decode function behind #[cfg(test)]. If non-test code ever needs to decode a bare ContributionAndProof (e.g., to add a proper deserialize branch as per the first finding), it will silently not compile in release builds.

Evidence: crates/core/src/ssz_codec.rs:125-130

Recommendation: Either make the function pub (not test-gated) or, if it is only ever needed in tests, add a comment explaining why.


[Medium] SignedBlockContents uses Vec<T> for blobs/KZG proofs — no length limit

deneb.rs:436-444 and electra.rs:295-303: kzg_proofs: Vec<KZGProof> and blobs: Vec<Blob> use plain Vec, which the derived Encode/Decode impls treat as SSZ lists with no maximum. The spec limit is MAX_BLOB_COMMITMENTS_PER_BLOCK = 4096. Oversized input could be silently accepted, consuming large amounts of memory during decode.

Evidence: crates/eth2api/src/spec/deneb.rs:436-444; crates/eth2api/src/spec/electra.rs:295-303

Recommendation: Use SszList<KZGProof, MAX_BLOB_COMMITMENTS_PER_BLOCK> and SszList<Blob, MAX_BLOB_COMMITMENTS_PER_BLOCK>.


[Medium] No SSZ fixture coverage for Altair–Fulu versioned types

The fixture generator (test-infra/sszfixtures/main.go) only produces Phase0 fixtures. Every versioned encode/decode function (Bellatrix, Capella, Deneb, Electra, Fulu proposals; Electra/Fulu attestations) is exercised only by hand-crafted round-trip tests, not by cross-language Go↔Rust byte-comparison tests.

Evidence: test-infra/sszfixtures/main.go:37-43

Recommendation: Add at least one fixture per fork for the most structurally distinct cases (e.g., Electra attestation with committee_bits, a Deneb SignedBlockContents with blobs, a Capella blinded proposal).


[Low] Compiled sszfixtures binary committed to git

test-infra/sszfixtures/sszfixtures is a tracked binary file (0-byte entry in the changed files list). Binaries in version control inflate repo size and can differ per architecture.

Evidence: test-infra/sszfixtures/sszfixtures (changed files listing)

Recommendation: Add it to .gitignore. The binary can always be rebuilt from go build . in the same directory.


[Low] decode_u8 accepts slices longer than 1 byte without error

decode.rs:6-13: decode_u8 checks only bytes.is_empty() but does not verify bytes.len() == 1. Callers passing a too-long slice will silently get bytes[0] back, discarding trailing data. This is inconsistent with decode_u32/decode_u64 which use try_into() and fail on wrong size.

Evidence: crates/ssz/src/decode.rs:6-13


[Low] BitList::with_bits and BitVector::with_bits do not validate bounds

types.rs:336-346 and types.rs:429-435: neither helper checks that the specified bit indices are within bounds, nor that capacity <= MAX for BitList. An out-of-bounds index will panic via bytes[bit / 8].

Evidence: crates/ssz/src/types.rs:336-346, crates/ssz/src/types.rs:429-435


Parity Matrix (wire format)

Component Go (Charon) fixture Rust fixture test Byte match Notes
phase0::Attestation raw SSZ attestation_phase0.ssz.hex go_fixture_attestation_phase0 verified
phase0::SignedAggregateAndProof raw SSZ signed_aggregate_and_proof.ssz.hex go_fixture_signed_aggregate_and_proof verified
VersionedAttestation Phase0 versioned_attestation_phase0.ssz.hex go_fixture_versioned_attestation_phase0 verified
VersionedSignedAggregateAndProof Phase0 versioned_agg_proof_phase0.ssz.hex go_fixture_versioned_agg_proof_phase0 verified
VersionedSignedProposal Phase0 versioned_proposal_phase0.ssz.hex go_fixture_versioned_proposal_phase0 verified
VersionedAttestation Electra hand-crafted only untested no Go fixture
VersionedSignedProposal Deneb (with blobs) not tested untested no fixture
Blinded proposals (Bellatrix–Fulu) not tested untested no fixture

Tests

Tests were not run (no build toolchain available). The Go-fixture compatibility tests are the primary cross-language verification. Round-trip tests are comprehensive for Phase0 types. See the Medium finding above about missing multi-fork fixture coverage.


Open Questions

  1. Is SyncContributionAndProof ever transmitted via parsigex in Charon? If not, the serialize path in parsigex_codec.rs:107-113 can be removed.
  2. Does DataVersion::Unknown ever reach encode_version? If the callers always guard against Unknown before calling, the error UnknownVersion(0) might never surface, but it's still a latent confusion.
  3. Should SszList<T, MAX=0> (zero max) be a valid type? The tree-hash semantics with minimum_leaf_count=0 may silently produce wrong roots for this case.

@iamquang95
Copy link
Copy Markdown
Collaborator

remember to remove golang code

@varex83 varex83 mentioned this pull request Apr 14, 2026
4 tasks
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