feat: add SSZ encoding/decoding support for parsigex#322
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @iamquang95's task in 4m 52s —— View job Code Review — feat: add SSZ encoding/decoding support for parsigex
SummaryThe 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 Findings[High]
If a peer ever serializes a Impact: The serialize path for Evidence: Recommendation: Either remove the [High]
Impact: Malformed or attacker-controlled SSZ input can produce a Evidence: Recommendation: After decoding, add: if MAX > 0 && value.len() > MAX {
return Err(DecodeError::BytesInvalid(format!(
"bitlist length {} exceeds max {MAX}", value.len()
)));
}[High] In Additionally, the method signature is Evidence: Recommendation: Change [Medium]
Evidence: Recommendation: Either store the [Medium]
Evidence: Recommendation: Either make the function [Medium]
Evidence: Recommendation: Use [Medium] No SSZ fixture coverage for Altair–Fulu versioned types The fixture generator ( Evidence: Recommendation: Add at least one fixture per fork for the most structurally distinct cases (e.g., Electra attestation with [Low] Compiled
Evidence: Recommendation: Add it to [Low]
Evidence: [Low]
Evidence: Parity Matrix (wire format)
TestsTests 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
|
|
remember to remove golang code |
|
Claude finished @varex83's task in 1m 15s —— View job Removed Go sszfixtures code
Removed |
The SSZ test fixtures have already been generated and committed under crates/core/testdata/ssz/. The Go generator code is no longer needed in-tree; the binary is already gitignored. Co-authored-by: Bohdan Ohorodnii <varex83@users.noreply.github.com>
|
@claude review |
|
Claude finished @varex83's task in 7m 24s —— View job Code Review — feat: add SSZ encoding/decoding support for parsigex
SummaryThe PR adds SSZ binary encode/decode support for Charon's parsigex signed-data types and is a solid foundation. Cross-language fixture tests for Phase0 types are a good signal. However, there are several issues ranging from a critical wire-format incompatibility with production Go Charon nodes to missing bounds checks in the Findings[Critical] In Go Charon (
In production, The Rust Go's The Go fixture Evidence: Recommendation: Add a [High]
The test for this path ( This confirms the Recommendation: Remove the [High]
Recommendation: After decoding, add: if MAX > 0 && value.len() > MAX {
return Err(DecodeError::BytesInvalid(format!(
"bitlist length {} exceeds max {MAX}", value.len()
)));
}[High]
Recommendation: Change [Medium]
Recommendation: Use [Medium]
[Medium]
pub kzg_proofs: Vec<KZGProof>, // → SszList<KZGProof, MAX_BLOB_COMMITMENTS_PER_BLOCK>
pub blobs: Vec<Blob>, // → SszList<Blob, MAX_BLOB_COMMITMENTS_PER_BLOCK>Note that changing these types requires removing the [Medium] No SSZ fixture coverage for Altair–Fulu versioned types The fixtures cover only Phase0. Every versioned encode/decode for Bellatrix, Capella, Deneb (with blobs), Electra/Fulu attestations, and all blinded proposals is only exercised by hand-crafted Rust round-trip tests — no cross-language byte-comparison verification. Particularly risky: the Recommendation: Add at least one Go-generated fixture per fork for the most structurally distinct cases (e.g., Electra [Low]
[Low]
[Low] Unrelated dev-dependencies added in The diff adds Parity Matrix
Open Questions
|
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM, the code is quite error prone due to repetition so I suggest taking a second look at Claude's output.
| publish.workspace = true | ||
|
|
||
| [dependencies] | ||
| ethereum_ssz = { workspace = true } |
There was a problem hiding this comment.
nit:
| ethereum_ssz = { workspace = true } | |
| ethereum_ssz.workspace = true |
| ethereum_ssz = { version = "0.10" } | ||
| ethereum_ssz_derive = { version = "0.10" } |
There was a problem hiding this comment.
We had some discussion in the past due to using multiple ssz libraries but I don't remember the details. Why are we adding this dependency?
There was a problem hiding this comment.
We add it to easily allow encoding/decoding of some structures via derive, we still can write derive from scratch, but I think this is a good use case. I think previous discussions were about tree hash
| #[error("{0}")] | ||
| Custom(String), |
There was a problem hiding this comment.
nit: We only use this for "invalid bool bytes", better to have a proper variant for that case.
| return Err(DecodeError::BytesInvalid(format!( | ||
| "vector length {} does not match required {SIZE}", | ||
| values.len(), | ||
| ))); |
There was a problem hiding this comment.
nit: There are better error variants that we can use:
| return Err(DecodeError::BytesInvalid(format!( | |
| "vector length {} does not match required {SIZE}", | |
| values.len(), | |
| ))); | |
| return Err(DecodeError::InvalidByteLength { | |
| len: values.len(), | |
| expected: SIZE, | |
| }); |
| version | ||
| .to_legacy_u64() | ||
| .map(encode_u64) | ||
| .map_err(|_| SszCodecError::UnknownVersion(0)) |
There was a problem hiding this comment.
Version Phase0 maps to 0 through to_legacy_u64. The error is misleading.
|
|
||
| /// Encodes a `VersionedAttestation` to SSZ binary with Charon versioned | ||
| /// header. | ||
| pub fn encode_versioned_attestation( |
There was a problem hiding this comment.
This seem not cover the same ssz in charon
// MarshalSSZTo ssz marshals the VersionedAttestation object to a target array.
func (a VersionedAttestation) MarshalSSZTo(dst []byte) ([]byte, error) {
version, err := eth2util.DataVersionFromETH2(a.Version)
if err != nil {
return nil, errors.Wrap(err, "invalid version")
}
if a.ValidatorIndex == nil {
return marshalSSZVersionedTo(dst, version, a.sszValFromVersion)
}
valIdx := *a.ValidatorIndex
return marshalSSZVersionedValidatorIdxTo(dst, version, valIdx, a.sszValFromVersion)
}
First need to check the ValidatorIndex to decide which ssz encoding needs to be used.
There was a problem hiding this comment.
func marshalSSZVersionedValidatorIdxTo(dst []byte, version eth2util.DataVersion, valIdx eth2p0.ValidatorIndex, valFunc func(eth2util.DataVersion) (sszType, error)) ([]byte, error) {
// Field (0) 'Version'
dst = ssz.MarshalUint64(dst, version.ToUint64())
// Field (1) 'ValidatorIndex'
dst = ssz.MarshalUint64(dst, uint64(valIdx))
// Offset (2) 'Value'
dst = ssz.WriteOffset(dst, versionedValIdxOffset)
// TODO(corver): Add a constant length data version string field, ensure this is backwards compatible.
val, err := valFunc(version)
if err != nil {
return nil, errors.Wrap(err, "sszValFromVersion from version")
}
// Field (2) 'Value'
if dst, err = val.MarshalSSZTo(dst); err != nil {
return nil, errors.Wrap(err, "marshal sszValFromVersion")
}
return dst, nil
}
It's 8+8+4 = 20 bytes if there is VersionIndex
There was a problem hiding this comment.
Also should have golden test for this (both case with and without VersionIndex)
|
@claude review it please |
|
Claude finished @varex83's task in 3m 39s —— View job Code Review — feat: add SSZ encoding/decoding support for parsigex
SummaryGood progress from the previous review — the critical Fixed ✅Critical → Fixed:
Medium → Fixed:
Still Open[High]
The round-trip test at Recommendation: Remove the [High]
fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, DecodeError> {
let value = Self::from_ssz_bytes(bytes.to_vec());
if MAX > 0 && value.len() > MAX {
return Err(DecodeError::BytesInvalid(format!(
"bitlist length {} exceeds max {MAX}", value.len()
)));
}
Ok(value)
}[High]
Recommendation: Change [Medium] Remaining
Recommendation: Use DataVersion::Unknown => Err(SszCodecError::UnknownVersion(u64::MAX))[Medium]
[Medium]
pub kzg_proofs: Vec<KZGProof>, // → SszList<KZGProof, MAX_BLOB_COMMITMENTS_PER_BLOCK>
pub blobs: Vec<Blob>, // → SszList<Blob, MAX_BLOB_COMMITMENTS_PER_BLOCK>[Medium] No SSZ fixture coverage for Altair–Fulu versioned types The fixture set covers only Phase0 (+ the new Phase0-with-validator_index). The [Low]
[Low]
Parity Matrix
|
|
@claude review |
|
Claude finished @varex83's task in 2m 38s —— View job Code Review — feat: add SSZ encoding/decoding support for parsigex
SummaryGood progress again — all the medium and low findings from the previous round have been addressed. Two high-severity Fixed ✅Low → Fixed: High → Fixed: Medium → Fixed: All remaining Nit → Fixed: Still Open[High]
fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, DecodeError> {
let value = Self::from_ssz_bytes(bytes.to_vec());
if MAX > 0 && value.len() > MAX {
return Err(DecodeError::BytesInvalid(format!(
"bitlist length {} exceeds max {MAX}", value.len()
)));
}
Ok(value)
}[High]
Recommendation: change [Medium]
// deneb.rs, electra.rs, fulu.rs
pub kzg_proofs: Vec<KZGProof>, // → SszList<KZGProof, MAX_BLOB_COMMITMENTS_PER_BLOCK>
pub blobs: Vec<Blob>, // → SszList<Blob, MAX_BLOB_COMMITMENTS_PER_BLOCK>[Medium] No Go fixtures for Altair–Fulu versioned types The fixture set covers only Phase0 (with and without [Low]
Parity Matrix
|
|
For these |
4 participants
Summary
crates/ssz/src/encode.rs,crates/ssz/src/decode.rs)crates/ssz/src/types.rswith SSZ type implementations for eth2api spec typescrates/eth2api/src/spec/*types (phase0, altair, bellatrix, capella, deneb, electra, fulu)crates/core/src/ssz_codec.rs— SSZ codec integration for parsigex signed dataparsigex_codec.rswith SSZ-based serialisation alongside existing JSON pathcrates/core/testdata/ssz/generated bytest-infra/sszfixtures/fastrand 2.4.0→2.4.1and ignores pre-existingrand 0.8.5advisory (RUSTSEC-2026-0097) present on base branchTest plan
cargo test --workspace --all-featurespassescargo clippy --workspace --all-targets --all-features -- -D warningspassescrates/coreexercise the new fixturescargo deny checkpasses🤖 Generated with Claude Code