refactor: extract from_parts on SSVMessage and SignedSSVMessage

[shane-moore]

Problem, Evidence, and Context

SSVMessage::new and SignedSSVMessage::new bundle two distinct concerns: SSZ-bounds construction and semantic validation (operator_ids non-zero/sorted/deduped, MsgType ↔ Data shape, etc.). Callers that need to construct a message at the on-wire size limit but bypass semantic invariants — e.g. spec-test fixtures that probe the SSZ encoder against intentionally-malformed-but-bounds-valid inputs — have no clean path today, leaving a duplication-or-private-fields choice.

Change Overview

• New pub fn from_parts(...) on both types: bounds-only construction (variable-length size checks via try_to_variable_list).
• new() becomes a one-liner: Self::from_parts(...)?.validate()? on both.
• Two stale internal comments narrating obvious code dropped.
• No behavior change for any existing caller of new() — same Result, same error variants in the same order (bounds first, then validate).

Risks, Trade-offs, and Mitigations

from_parts is a lower-guarantee constructor than new() — production code should keep using new(). The doc comment on each from_parts references validate so the bypass is visible at the call site. No production callers of from_parts are added in this PR.

Validation

• cargo test -p ssv_types --release: 80 unit tests + 2 doctests pass
• make cargo-fmt-check clean
• make lint clean for the touched crate
• Existing new() callers exercise the same from_parts → validate path post-refactor

Rollback

Single-file revert. No data, config, or runtime impact.

Additional Info / Next Steps

Splits out of the test/structure-size-test branch so this small refactor can land independently. The follow-up spec-test PR (which adds maxmsgsize.StructureSizeTest) consumes from_parts to encode max-size SSV/SignedSSV fixtures whose Go-side construction intentionally violates Anchor's stricter validate() — e.g., zero-padded OperatorIDs to reach max wire size, or oversized Data against MsgType.

[shane-moore]

@claude, please take a look

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 43s —— View job

---

Code Review: from_parts on SSVMessage and SignedSSVMessage

• Read the changed file and diff
• Analyze the implementation
• Post review feedback

---

Verdict: LGTM — clean, mechanical refactor with no behavioral change for existing callers.

The decomposition is sound: from_parts handles bounds enforcement (SSZ variable-list capacity), validate handles semantic invariants, and new composes both. The new() = from_parts()? + validate()? pattern preserves the existing error ordering (bounds first, semantics second), so no caller behavior changes.

One minor observation — not blocking:

SSVMessage::from_parts allows empty data (message.rs:221-234). This is intentional per the design (bounds-only, no semantics), but it means a caller of from_parts can construct an SSVMessage with empty data that would fail validate(). The doc comment "Bounds-checked construction without semantic validation; see Self::validate" makes this clear enough. The follow-up spec-test use case (intentionally malformed fixtures) validates this design choice.

No correctness, security, or performance issues found.

---

[diegomrsantos]

I do not think we should add a public constructor that can create semantically invalid SSVMessage or SignedSSVMessage values from ordinary Rust inputs.

The SSZ decode path is already necessarily unvalidated because decoding has to happen before semantic validation. But that does not mean we should add another public construction path that bypasses validate(). new() is the constructor that preserves the expected invariants, and from_parts() makes it easy for production code to accidentally work with values that validate() would reject.

The spec-test motivation makes sense, but I think the workaround should stay outside the production constructor API: build raw SSZ fixture bytes in the spec-test crate, add a clearly test-only helper, or introduce a separate unchecked/raw fixture type. I would prefer not to expose this on the production SSVMessage / SignedSSVMessage API.

[shane-moore]

Fair point on production API surface — agreed. Will gate from_parts behind a spec-test feature on ssv_types (matching the existing arbitrary-fuzz precedent), enabled only by the spec_tests crate. That keeps the default public API as new()-only while still letting the structure-size test exercise the production encoder directly rather than a copy, i.e.

impl SSVMessage {
    pub fn new(
        msg_type: MsgType,
        msg_id: MessageId,
        data: Vec<u8>,
    ) -> Result<Self, SSVMessageError> {
        let ssv_message = Self::build_unvalidated(msg_type, msg_id, data)?;
        ssv_message.validate()?;
        Ok(ssv_message)
    }

    /// Bounds-only construction. Reachable from outside the crate only with
    /// the `spec-test` feature; spec fixtures (`maxmsgsize.StructureSizeTest`)
    /// build max-size values whose contents intentionally violate `validate`.
    /// Production callers must use `new`, which always validates.
    #[cfg(feature = "spec-test")]
    pub fn from_parts(
        msg_type: MsgType,
        msg_id: MessageId,
        data: Vec<u8>,
    ) -> Result<Self, SSVMessageError> {
        Self::build_unvalidated(msg_type, msg_id, data)
    }

    fn build_unvalidated(
        msg_type: MsgType,
        msg_id: MessageId,
        data: Vec<u8>,
    ) -> Result<Self, SSVMessageError> {
        let data = try_to_variable_list::<u8, SSVMessageDataLen, _, _>(data, |provided, max| {
            SSVMessageError::SSVDataTooBig { provided, max }
        })?;
        Ok(SSVMessage {
            msg_type,
            msg_id,
            data,
        })
    }

    pub fn validate(&self) -> Result<(), SSVMessageError> { /* unchanged */ }
}

reason for this approach besides keeping the default public surface clean: this is essentially your option 2 (clearly test-only helper) in concrete form. The symbol only exists when the spec-test feature is on, the feature name explicitly labels it test-only, and it matches the arbitrary-fuzz pattern already on this file. If you had a different shape in mind for option 2, let me know.

Options 1 and 3 we'd want to avoid because neither actually exercises the production encoder

[diegomrsantos]

I opened #1009 to track the broader type-design issue here: raw SSZ-shaped messages and validated protocol messages are currently represented by the same Rust types.

For this PR, I do not think we need to solve that full refactor. The immediate thing I would avoid is adding an inherent constructor on SSVMessage / SignedSSVMessage that returns values which may fail validate(). A small test-only helper is fine if the follow-up spec test needs it, but I would prefer the helper to make the escape hatch explicit and, ideally, return encoded bytes rather than handing an invalid production message object back to the caller.

The long-term direction in #1009 is to introduce raw wire-container types, so SSZ decoding and max-size fixture construction can work with raw messages while normal production code receives validated message types.

[shane-moore]

Thanks for opening #1009 — that's the right home for this.

Rather than implement the bytes-returning helper, I'd like to defer the SSVMessage / SignedSSVMessage parts of StructureSizeTest until #1009 lands. 6 of the 8 fixture types in that test already exercise the production encoder directly; only those two cases need an alternative path because their fixtures intentionally fail validate(). The bytes-helper would mean those two assertions run against a parallel encoder rather than production — at that point we're not really testing production for those types, and the helper gets deleted once RawSSVMessage / RawSignedSSVMessage exist anyway.

Concretely:

1. Close this PR — without the structure-size consumer the change has no remaining purpose.
2. Land the structure-size runner with the 6 working types; max_SSVMessage* / max_SignedSSVMessage* skipped with a // TODO(#1009). Re-enabling them is a one-line change once the raw types exist.

Wdyt?