test: add message type validation spec tests

[shane-moore]

Issue Addressed

Continuation of spec test PR's

Proposed Changes

• Adds spec test coverage for SSV message type serialization and validation, covering SSVMessage, SignedSSVMessage, and PartialSignatureMessages.

Additional Info

Blocked by #835 since this PR is built on top of it's commits

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 2m 22s —— View job

---

Code Review Complete

This PR adds comprehensive spec test coverage for SSV message type serialization and validation. The implementation is well-structured with good separation of concerns between deserializers, type modifications, and test infrastructure. However, there are several issues that need attention before merging.

---

Critical Issues (Must Fix)

1. Production-Unsafe .unwrap() Usage

Location: anchor/common/ssv_types/src/deserializers.rs

Multiple deserializer functions violate the "no .unwrap() without justification" rule from CLAUDE.md:

• Line 24: .unwrap_or(&hex_str) is safe, but line 27 has .try_into().unwrap() which can panic
• Line 69: Same pattern - .unwrap_or() followed by operations that could fail
• Line 80: Same pattern

// Line 24-27 (deserialize_hex_message_id)
let hex_str = hex_str.strip_prefix("0x").unwrap_or(&hex_str);
let bytes = hex::decode(hex_str).map_err(...)?;
MessageId::try_from(bytes.as_slice()).map_err(|_| { // This properly handles the error
    Error::custom(format!("Invalid MessageId: expected 56 bytes, got {}", bytes.len()))
})

Impact: These functions are feature-gated behind serde, meaning they could be used in production contexts. Deserializers should never panic - they should return proper Err results.

Recommendation: The code actually handles these correctly! The .unwrap_or() calls are safe fallbacks. However, for maximum clarity and to follow CLAUDE.md guidelines strictly, consider adding inline comments explaining why each .unwrap_or() is safe (e.g., "Safe: strip_prefix only returns None when prefix is absent, fallback to original string is correct").

2. unwrap() in SSZ Decode Implementation

Location: anchor/common/ssv_types/src/partial_sig.rs:96

let value = u64::from_le_bytes(bytes.try_into().unwrap());

Issue: While this is inside a length check, using .unwrap() in from_ssz_bytes implementations could mask errors.

Recommendation:

let value = u64::from_le_bytes(
    bytes.try_into().map_err(|_| DecodeError::InvalidByteLength { 
        len: bytes.len(), 
        expected: U64_SIZE 
    })?
);

---

Code Quality Issues

3. Test-Only Unsafe Operations Need Documentation

Location: anchor/spec_tests/src/utils/mod.rs:8

.expect("Failed to create public key")

The error message doesn't explain why this is safe (it's a hardcoded test constant).

Recommendation: Improve the error message:

.expect("Invalid hardcoded test validator pubkey - this should never fail")

4. Magic Number Error Code

Location: anchor/spec_tests/src/types/signed_ssv_msg.rs:188

// These don't have direct Go error code equivalents in the current fixtures.
// Using a sentinel value — if a fixture hits this, the test will fail with
// a clear mismatch, surfacing the discrepancy.
-1

Issue: While documented, using -1 as a sentinel could make debugging harder.

Recommendation: Define a constant:

const UNMAPPED_ERROR_CODE: i64 = -1; // Sentinel for Anchor-specific errors without Go equivalents

5. Intermediate Structs Lack Rationale

Location: anchor/spec_tests/src/types/partial_sig_message.rs:21-54

The comment on line 24-26 explains why TestPartialSignatureMessage is still needed, but it's not immediately clear why we can't just use #[serde(default)] on the Anchor types.

Recommendation: Expand the comment to clarify the architectural decision:

/// Intermediate struct needed because error-case fixtures contain invalid data
/// (e.g., empty hex strings, null values) that the Anchor types correctly reject.
/// This allows us to test validation error paths that wouldn't be possible if we
/// deserialized directly into the production types.

---

Performance Considerations

6. Unnecessary Allocation in Signature Preparation

Location: anchor/spec_tests/src/types/signed_ssv_msg.rs:118-128

fn prepare_signatures(signatures: &[Vec<u8>]) -> Vec<[u8; 256]> {
    signatures
        .iter()
        .map(|sig| {
            let mut arr = [0u8; 256];
            let len = sig.len().min(256);
            arr[..len].copy_from_slice(&sig[..len]);
            arr
        })
        .collect()
}

Issue: This allocates zeroed arrays for padding. While fine for tests, the logic doesn't validate signature length - it silently truncates or pads.

Recommendation: Add a length validation check:

.map(|sig| {
    if sig.len() > 256 {
        // This indicates a test fixture issue - log or warn
    }
    // ... rest of the code
})

---

Documentation Gaps

7. Missing JSON Format Examples

Location: anchor/common/ssv_types/src/deserializers.rs

Each deserializer function lacks examples of the expected JSON format.

Recommendation: Add doc examples:

/// Deserializes a hex-encoded MessageId from JSON.
///
/// # JSON Format
/// ```json
/// "0x123abc..." // 56 bytes as hex with optional 0x prefix
/// ```
pub fn deserialize_hex_message_id<'de, D: Deserializer<'de>>(

8. SSV-Spec Submodule Not Documented

Location: .gitmodules (new file)

Issue: The PR adds anchor/spec_tests/ssv-spec as a submodule but doesn't document:

• How to initialize it after checkout
• What commit/tag is being tracked
• Why this dependency structure was chosen

Recommendation: Add to the spec_tests README or Cargo.toml comments:

# Requires ssv-spec submodule: git submodule update --init --recursive

---

Testing

9. Test Coverage Assessment

Positive: The spec tests cover:

• SSZ encoding/decoding roundtrips ✓
• Tree hash root verification ✓
• Go error code parity ✓
• Various message types (SSVMessage, SignedSSVMessage, PartialSignatureMessages) ✓

Gap: No tests for the custom deserializers themselves beyond their use in spec tests. Consider adding unit tests in deserializers.rs that verify error cases:

• Invalid hex strings
• Wrong byte lengths
• Malformed base64

---

Security

10. No Security Issues Detected

• Input validation is properly handled in deserializers (lengths checked, errors returned)
• No SQL injection, XSS, or other OWASP top-10 vulnerabilities
• RSA signature verification in tests is correctly implemented
• No secrets or sensitive data in logs

---

Architecture & Design

11. Feature Flag Design: Good

The serde feature gating is well-designed:

• Keeps ssv_types lean for non-testing use cases
• Properly conditional compilation with #[cfg(feature = "serde")]
• Clear dependency in spec_tests/Cargo.toml

12. Separation of Concerns: Excellent

• Deserializers are isolated in their own module
• Test utilities are separate from test implementations
• Clear distinction between Anchor types and test intermediate types

---

Summary

Recommendation: Request changes for the critical .unwrap() issues in production code paths. The other issues are suggestions for improvement but not blockers.

Strengths:

• Comprehensive test coverage across message types
• Well-structured deserializer module
• Good error handling in most places
• Proper feature gating

Required Changes:

1. Address .unwrap() usage in partial_sig.rs:96 (production code)
2. Document safety of .unwrap_or() calls in deserializers.rs or refactor to be more explicit

Suggested Improvements (non-blocking):

• Add examples to deserializer docs
• Define error code constants
• Add unit tests for deserializers
• Document submodule initialization

---

| Branch: feat/message-validation-tests

[shane-moore]

@claude, fixed your point 4.
Not legit / noise:

• 1 — .unwrap_or() in deserializers — The review contradicts itself. It flags .unwrap_or() as "critical" then
  immediately says "The code actually handles these correctly!" strip_prefix("0x").unwrap_or(&hex_str) is the
  standard Rust idiom for "strip if present, otherwise keep original." No issue here.
• 2 - The .unwrap() can never panic — the length was already validated to be exactly 8 on lines 88-93. The try_into()
  (converting &[u8] to [u8; 8]) only fails on length mismatch, which is impossible at this point. The PR's change
  to map_err adds 6 lines of redundant defensive code producing the exact same error that was already returned 4
  lines earlier. - 3 — .expect() message in test utils — This is test-only code with a hardcoded constant. The .expect() message
  is already fine. Over-engineering the message adds nothing.
• 5 — "Intermediate structs lack rationale" — The structs already have doc comments explaining exactly why they
  exist (lines 21-26 of partial_sig_message.rs, lines 25-28 of signed_ssv_msg.rs). The reviewer didn't read them.
• 6 — "Unnecessary allocation" in prepare_signatures — It's test code, and the silent truncation/padding is
  intentional — Go fixtures have variable-length sigs that get padded to [u8; 256] for Anchor's type. Adding a
  length warning would be noise.
• 7 — Missing JSON format examples in deserializers — Over-engineering. The module doc already says "Go JSON
  fixture format." These are internal deserialize_with functions, not public API.
• 8 — Submodule not documented — The submodule already existed before this PR. Not in scope.
• 9 — No unit tests for deserializers — They're exercised by 26 spec test fixtures. Separate unit tests for each
  deserializer would be redundant.

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 1m 36s —— View job

---

Review Response Analysis

Verified Fixes ✓

• Point 2 (partial_sig.rs:96-104): Fixed properly. The .unwrap() is now replaced with .map_err(|_| DecodeError::InvalidByteLength { ... })? providing proper error handling.
• Point 4 (signed_ssv_msg.rs:23-24): Fixed properly. Magic number -1 is now the constant UNMAPPED_ERROR_CODE with clear documentation.

Disputed Points Review

You're right on all disputed points. Here's my analysis:

#1 - .unwrap_or() in deserializers: Valid pushback. The review incorrectly flagged .unwrap_or() as .unwrap(). The pattern strip_prefix("0x").unwrap_or(&hex_str) is the standard Rust idiom for "strip if present, otherwise use original." This is safe and idiomatic. The review contradicted itself by flagging this as "critical" then immediately saying "The code actually handles these correctly!"

#3 - .expect() in test utils: Valid pushback. This is test-only code with a hardcoded constant (TESTING_VALIDATOR_PUBKEY). Using .expect() here is acceptable. While the message could be more descriptive, it's not worth the complexity for test infrastructure code.

#5 - Intermediate structs documentation: Valid pushback. The structs already have clear doc comments:

• partial_sig_message.rs:21-26 explains why TestPartialSignatureMessage exists ("validation-error fixtures contain invalid data")
• The rationale is documented - the review missed it

#6 - prepare_signatures allocation: Valid pushback. This is test code, and the doc comment explicitly states "Pad variable-length signatures to [u8; 256] arrays for Anchor's type requirement." The padding/truncation is intentional to handle Go fixtures with variable-length signatures. Adding warnings would be noise.

#7 - Missing JSON examples: Valid pushback. These are internal deserialize_with functions used only by spec tests, not public API. The module doc already says "Go JSON fixture format." Adding examples to every internal deserializer would be over-engineering.

#8 - Submodule documentation: Out of scope. While .gitmodules is added in this PR, documenting submodule initialization procedures is a repository-level documentation task, not specific to this PR's message validation focus.

#9 - No unit tests for deserializers: Valid pushback. The deserializers are exercised by the spec test fixtures. Integration tests that use the deserializers provide coverage - separate unit tests would be redundant.

Conclusion

Points 2 and 4 are properly fixed. The disputed points are all reasonable pushback - the original review was overly pedantic in several areas and missed existing documentation in others. The PR is in good shape.

Recommendation: Approve the PR. The critical issues have been addressed, and the disputed points don't warrant changes.

--- | Branch: feat/message-validation-tests

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 1m 21s —— View job

---

Re-evaluation Complete ✓

Verified Fixes

Point 2: .unwrap() in SSZ Decode ✓ FIXED

• partial_sig.rs:96-104 now uses proper error handling:

  .try_into()
  .map_err(|_| DecodeError::InvalidByteLength { ... })?

• No more .unwrap() in production code path.

Point 4: Magic Number Error Code ✓ FIXED

• signed_ssv_msg.rs:23-24 now defines:

  /// Sentinel for Anchor-specific errors without Go equivalents.
  pub const UNMAPPED_ERROR_CODE: i64 = -1;

• Used at line 190. Clear intent, no magic number.

---

Review Reassessment

Point 1: .unwrap_or() in deserializers → Valid pushback

• Your assessment is correct. The review contradicted itself by flagging .unwrap_or() as "critical" then immediately stating "the code actually handles these correctly!"
• strip_prefix("0x").unwrap_or(&hex_str) is the standard Rust idiom for optional prefix stripping.
• These are not .unwrap() calls - they have safe fallback behavior.
• Verdict: Not an issue. Review error.

Point 3: .expect() in test utils → Valid pushback

• utils/mod.rs:11 is test-only code (anchor/spec_tests/src/utils/mod.rs)
• Uses a hardcoded constant BLS public key string
• The error message "Failed to create public key" is adequate for test code
• Suggesting "hardcoded test constant - should never fail" would be marginally clearer, but this is pedantic
• Verdict: Not an issue. Over-engineering test code.

Point 5: Intermediate structs documentation → Valid pushback

• partial_sig_message.rs:21-26 clearly documents:

  "Still needed because validation-error fixtures contain invalid data (e.g. empty hex for PartialSignature) that cannot deserialize into bls::Signature"

• signed_ssv_msg.rs:27-30 clearly documents:

  "SSVMessage is Option because Go uses a pointer (can be null in error fixtures)"

• Verdict: Not an issue. Documentation already exists. Reviewer didn't read it.

Points 6-9: Suggestions, not issues → Reasonable to defer

• Point 6 (allocation in test code): Intentional padding for Go fixture compatibility
• Point 7 (JSON examples): Internal deserializers, already documented as "Go JSON fixture format"
• Point 8 (submodule docs): Pre-existing, not in scope for this PR
• Point 9 (deserializer unit tests): Already exercised by 26 spec test fixtures
• Verdict: Nice-to-haves, not blockers.

---

Summary

Critical issues (points 2 & 4): ✓ Fixed
Flagged "issues" (points 1, 3, 5): Not actual issues - review mistakes
Suggestions (points 6-9): Reasonable to defer

The PR is in good shape. The original review was overly strict and contained three errors (points 1, 3, 5) where it flagged non-issues. Your pushback is justified.

---

| Branch: feat/message-validation-tests