fix(error handling) - Do not ignore msg validation errors, refactor

[oleg-ssvlabs]

Summary

Refactor QBFT justification decoding to stop suppressing extraction errors and centralize conversion of
justification messages into ProcessingMessages.

Problem

Several QBFT call sites were extracting justifications like this:

• call GetRoundChangeJustifications() / GetPrepareJustifications()
• ignore the returned error
• loop over []*types.SignedSSVMessage
• convert each item into *ProcessingMessage

The current code path relied on earlier Validate() calls to catch malformed justification bytes, so this was not
an active validation bypass today. But the error handling was non-local and brittle, and the conversion logic was
duplicated across proposal and round-change flows.

Changes

• added shared helpers on qbft.Message:
  • RoundChangeJustificationProcessingMessages()
  • PrepareJustificationProcessingMessages()
• added an internal helper to convert []*types.SignedSSVMessage into []*ProcessingMessage
• updated proposal and round-change call sites to use the shared helpers
• removed ignored justification extraction errors at those call sites
• kept the successful-path behavior unchanged:
  • same justification source fields
  • same item order
  • same NewProcessingMessage(...) conversion path

Why

This makes the invariant local at the point of use:

• malformed justifications now return explicit errors instead of relying on prior validation elsewhere
• the repeated decode/wrap loops are centralized
• future refactors are less likely to accidentally reintroduce silent error suppression

Tests

Added focused QBFT unit tests covering:

• malformed round-change justification payloads return a decode error
• valid prepare justifications are converted into ProcessingMessages successfully

Closes https://github.com/ssvlabs/ssv-node-board/issues/845
Closes https://github.com/ssvlabs/ssv-node-board/issues/843

[greptile-apps]

Greptile Summary

This PR refactors QBFT justification handling by introducing two new helper methods (RoundChangeJustificationProcessingMessages and PrepareJustificationProcessingMessages) on qbft.Message, backed by a shared internal converter (processingMessagesFromSignedMessages). All four previously duplicated decode-and-wrap loops across proposal.go and round_change.go are replaced with single-line calls to these helpers, and the previously silenced errors from GetRoundChangeJustifications() / GetPrepareJustifications() are now explicitly propagated.

• Behavioral correctness: The successful-path behavior is unchanged — same source fields, same item order, same NewProcessingMessage conversion. The only observable behavioral difference is that decode errors are now surfaced at these call sites rather than being masked (which was safe before due to earlier Validate() checks, but is now locally correct as well).
• Side-effect fix: The old proposal.go loop used msg as a loop variable, silently shadowing the outer *ProcessingMessage receiver parameter. The new code eliminates this shadowing.
• Tests: Two unit tests are added covering an error path and a success path for the new helpers. Coverage is asymmetric — the symmetric error/success cases for each method are not tested (minor, since both methods share the same internal helper).
• Naming: The new method names (RoundChangeJustificationProcessingMessages, PrepareJustificationProcessingMessages) are descriptive but verbose relative to the repo's naming conventions.

Confidence Score: 5/5

Safe to merge — no P0 or P1 issues found; all remaining feedback is style and test-coverage suggestions.

All functional changes are pure refactors that preserve the existing behavior on the happy path and strictly improve error propagation. The two P2 comments (verbose naming, asymmetric test coverage) are non-blocking quality suggestions. No data integrity, security, or correctness issues were identified.

No files require special attention.

Important Files Changed

Filename	Overview
qbft/messages.go	Adds two new public helpers (RoundChangeJustificationProcessingMessages, PrepareJustificationProcessingMessages) and one internal helper (processingMessagesFromSignedMessages) to centralize the decode→wrap conversion logic; implementation is correct and clean.
qbft/messages_test.go	New test file covering the error path for RoundChangeJustificationProcessingMessages and the success path for PrepareJustificationProcessingMessages; missing the symmetric cases (success for RoundChange, error for Prepare).
qbft/proposal.go	Replaces duplicated inline decode loops with the new helper methods; also eliminates the pre-existing local variable shadowing of the outer msg parameter in the prepare loop.
qbft/round_change.go	Three separate call sites (uponRoundChange, hasReceivedProposalJustificationForLeadingRound, validRoundChangeForDataIgnoreSignature) are refactored to use the new helpers; ignored errors are now propagated correctly.

Sequence Diagram

sequenceDiagram
    participant caller as proposal.go / round_change.go
    participant msg as qbft.Message
    participant helper as processingMessagesFromSignedMessages
    participant npm as NewProcessingMessage

    caller->>msg: RoundChangeJustificationProcessingMessages()
    msg->>msg: GetRoundChangeJustifications()
    msg-->>caller: error (propagated, no longer silenced)
    msg->>helper: processingMessagesFromSignedMessages(signedMsgs)
    loop for each SignedSSVMessage
        helper->>npm: NewProcessingMessage(signedMsg)
        npm-->>helper: *ProcessingMessage or error
    end
    helper-->>msg: []*ProcessingMessage or error
    msg-->>caller: []*ProcessingMessage or error

    caller->>msg: PrepareJustificationProcessingMessages()
    msg->>msg: GetPrepareJustifications()
    msg-->>caller: error (propagated, no longer silenced)
    msg->>helper: processingMessagesFromSignedMessages(signedMsgs)
    loop for each SignedSSVMessage
        helper->>npm: NewProcessingMessage(signedMsg)
        npm-->>helper: *ProcessingMessage or error
    end
    helper-->>msg: []*ProcessingMessage or error
    msg-->>caller: []*ProcessingMessage or error

Loading

_{Reviews (1): Last reviewed commit: "Do not ignore msg validation errors, ref..." | Re-trigger Greptile}

[GalRogozinski]

we have qbft/spectest/tests/messages/round_change_justification_unmarshaling.go and qbft/spectest/tests/messages/prepare_justification_unmarshaling.go

what coverage do you add here?

[oleg-ssvlabs]

From what I can see, the existing spec tests cover unmarshalling justification bytes into []*SignedSSVMessage (via GetRoundChangeJustifications / GetPrepareJustifications called in Validate()). The new helper methods add a second step, converting []*SignedSSVMessage into []*ProcessingMessage by decoding SSVMessage.Data into a qbft.Message. The existing tests don't cover that path. The new unit tests cover both success and error cases for that conversion.

[iurii-ssv]

LGTM

[momosh-ssv]

Non-blocking observations — the refactor itself looks correct and I don't see any behavioral regression (the ignored errors were already gated upstream by msg.Validate()).

[momosh-ssv]

Might be worth renaming prepareMsgs here (or adding a one-line comment). The source is RoundChangeJustificationProcessingMessages() but the variable is named prepareMsgs — correct per spec (RoundChange packs the prepare quorum into RoundChangeJustification), but the name/source mismatch is easy to misread. Pre-existing, but this was a natural moment to clarify.