fix(validation): Check the signatures of the justifications' justifications

[dknopik]

Issue Addressed

Messages used in round change justifications can have justifications themselves. Check those.

Proposed Changes

Decode the justifications and call validate_justifications on them.

[Zacholme7]

nice!

[Zacholme7]

Love recursion in the wild

[diegomrsantos]

is validate_justifications validating only signatures?

[diegomrsantos]

Prepare messages never carry justifications. So recursing “inside a Prepare” is a dead end by spec design

[Zacholme7]

it will only recurse if it is non-empty, and since prepare messages never carry justifications it will not recurse

[diegomrsantos]

Imo this isn't a good idea

[diegomrsantos]

For example, if someone adds, for some reason, invalid justifications inside a prepare message, our code will reject it, but it's "valid". If we want to be very strict, imo it'd be better to assert that a prepare message doesn't have any justification instead.

[diegomrsantos]

How about something like

pub(crate) fn validate_justifications(
    consensus_message: &QbftMessage,
    operator_pub_keys: &HashMap<OperatorId, Rsa<Public>>,
) -> Result<(), ValidationFailure> {
    // ----- Presence rules (where each justification set is allowed) -----
    // Prepare justifications MUST appear only on Proposals.
    if !consensus_message.prepare_justification.is_empty()
        && consensus_message.qbft_message_type != QbftMessageType::Proposal
    {
        return Err(ValidationFailure::UnexpectedPrepareJustifications);
    }

    // "round_change_justification" is:
    // - on a Proposal: the RC set (each item must be a RoundChange)
    // - on a RoundChange: the prepares for the last prepared (each item must be a Prepare)
    // - otherwise: must be empty
    if !consensus_message.round_change_justification.is_empty()
        && consensus_message.qbft_message_type != QbftMessageType::Proposal
        && consensus_message.qbft_message_type != QbftMessageType::RoundChange
    {
        return Err(ValidationFailure::UnexpectedRoundChangeJustifications);
    }

    // ----- Type + signature checks for each set -----
    match consensus_message.qbft_message_type {
        QbftMessageType::Proposal => {
            // On Proposal:
            //   - round_change_justification := set of RoundChange messages
            //   - prepare_justification      := set of Prepare messages (the single prepared cert)
            for signed in &consensus_message.round_change_justification {
                let inner = decode_inner_qbft(signed)?;
                if inner.qbft_message_type != QbftMessageType::RoundChange {
                    return Err(ValidationFailure::JustificationWrongType {
                        expected: QbftMessageType::RoundChange,
                        got: inner.qbft_message_type,
                    });
                }
                verify_message_signatures(signed, operator_pub_keys)?;
                // IMPORTANT: do NOT recurse into inner.justifications here.
                // The spec does not require per-RC nested-prepare validation on proposal validation.
            }

            for signed in &consensus_message.prepare_justification {
                let inner = decode_inner_qbft(signed)?;
                if inner.qbft_message_type != QbftMessageType::Prepare {
                    return Err(ValidationFailure::JustificationWrongType {
                        expected: QbftMessageType::Prepare,
                        got: inner.qbft_message_type,
                    });
                }
                verify_message_signatures(signed, operator_pub_keys)?;
                // No recursion; prepares never carry justifications per spec model.
            }
        }

        QbftMessageType::RoundChange => {
            // On RoundChange:
            //   - prepare_justification MUST be empty (already enforced above)
            //   - round_change_justification := set of Prepare messages proving "prepared" (if prepared)
            for signed in &consensus_message.round_change_justification {
                let inner = decode_inner_qbft(signed)?;
                if inner.qbft_message_type != QbftMessageType::Prepare {
                    return Err(ValidationFailure::JustificationWrongType {
                        expected: QbftMessageType::Prepare,
                        got: inner.qbft_message_type,
                    });
                }
                verify_message_signatures(signed, operator_pub_keys)?;
            }
        }

        // For Prepare / Commit / others, both sets must be empty (already enforced above).
        _ => { /* nothing else to validate */ }
    }

    Ok(())
}

/// Decode the embedded QbftMessage out of a SignedMessage’s SSZ bytes.
fn decode_inner_qbft(signed: &SignedMessage) -> Result<QbftMessage, ValidationFailure> {
    QbftMessage::from_ssz_bytes(signed.ssv_message().data())
        .map_err(|_| ValidationFailure::MalformedJustifications)
}

[Zacholme7]

If we are confident on not checking the inner justifications I think that looks nice

[diegomrsantos]

For a proposal, we don't need to check the prepares inside the round change

[diegomrsantos]

But for a standalone round change, we actually need to

[diegomrsantos]

is this supported by the spec?

[Copilot]

Pull Request Overview

This PR implements recursive validation of justifications within consensus messages by checking the signatures of nested justifications. The change ensures that when messages contain justifications (such as in round change scenarios), those justifications' own justifications are also validated recursively.

• Consolidates validation error types by removing duplicate error variants
• Adds recursive validation of nested justifications in consensus messages
• Updates error handling to use the consolidated MalformedJustifications error type

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
anchor/message_validator/src/lib.rs	Consolidates error types by removing duplicate justification validation errors
anchor/message_validator/src/consensus_message.rs	Implements recursive validation of justifications and updates error handling

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

This recursive call creates potential for infinite recursion if justifications contain circular references. Consider adding a recursion depth limit or cycle detection to prevent stack overflow.

[Zacholme7]

Im not sure this is possible. We cap the data lengths and I think circular references are hard to create in rust without some fancy RefCell stuff?

[diegomrsantos]

Let's confirm first this is supported by the spec? Is go doing it?

[diegomrsantos]

how about https://leastauthority.com/wp-content/uploads/2024/01/Coin_Dash_Ltd._SSV_Specification_Final_Audit_Report_Updated_Least-Authority-.pdf

[Zacholme7]

A message with an invalid signature is not something we want to accept

[jking-aus]

I'm good for this to go in. Tag it to merge when you're ready @dknopik

Removing the request