feat: Majority Fork Protection

[dknopik]

Issue Addressed

• Majority Fork Protection #628

Proposed Changes

In the BeaconVoteValidator, which validates BeaconVotes proposed by other nodes, add a step to compare the source, and a step to compare the target, including epoch and checkpoing for each.

Additional Info

Implements ssvlabs/ssv-spec#555.

See also the Go-SSV implementation: ssvlabs/ssv#2358

[claude-code-actions-sigp]

Claude finished @dknopik's task —— View job

---

Code Review Complete

I've reviewed the Majority Fork Protection implementation. Here's my detailed analysis:

Critical Bug Found 🐛

Lines 689-692 in anchor/common/ssv_types/src/consensus.rs:

if value.target != our_value.target {
    return Err(BeaconVoteValidationError::DifferentTarget {
        our: our_value.source,      // ❌ BUG: Should be our_value.target
        proposed: value.source,      // ❌ BUG: Should be value.target
    });
}

The error construction uses source instead of target in both fields. This is a copy-paste error from the source validation above. The error message will display incorrect checkpoint information, making debugging nearly impossible.

Fix:

if value.target != our_value.target {
    return Err(BeaconVoteValidationError::DifferentTarget {
        our: our_value.target,
        proposed: value.target,
    });
}

Code Quality Assessment

Positive aspects:

• Implementation correctly validates source and target checkpoints match between operators
• Error types are well-defined with descriptive messages
• Follows existing code patterns in the validator
• Removes the unused parameter prefix (_our_value → our_value)
• Security-critical validation occurs before slashing protection checks

Issues identified:

1. Missing test coverage - No tests validate the new source/target comparison logic. The existing tests in anchor/qbft_manager/src/tests.rs only use default checkpoints, meaning the new validation paths are never exercised.

2. No documentation - The PR doesn't document why this validation is critical for majority fork protection or reference the spec requirement.

Security Analysis ✓

The implementation adds important Byzantine fault tolerance by preventing operators from proposing different source/target checkpoints. This protects against:

• Fork choice manipulation
• Validators attesting to conflicting chain views
• Majority of operators colluding to create invalid attestations

The validation correctly occurs before slashing protection checks, ensuring invalid proposals are rejected early.

Performance ✓

No performance concerns. The checkpoint comparisons are simple equality checks on fixed-size structures.

Recommendations

1. Fix the critical bug immediately - The incorrect error fields must be corrected
2. Add comprehensive tests covering:
   • Mismatched source checkpoints (different epochs)
   • Mismatched source checkpoints (same epoch, different roots)
   • Mismatched target checkpoints (different epochs)
   • Mismatched target checkpoints (same epoch, different roots)
   • Valid matching checkpoints (should pass)
3. Add inline documentation explaining the fork protection mechanism
4. Reference the SSV spec issue fix: properly justify round change #555 in comments for future maintainers

Verdict

The implementation approach is sound and addresses the security issue correctly, but contains a critical bug in error reporting and lacks test coverage for the new validation logic.

Action required: Please fix the bug and add tests before merging.

---

[diegomrsantos]

@claude review this PR

[claude-code-actions-sigp]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[diegomrsantos]

@claude review this PR

[claude-code-actions-sigp]

Claude finished @diegomrsantos's task —— View job

---

Code Review In Progress

Tasks

• Read modified file and examine current implementation
• Check if critical bug from previous review was fixed
• Verify PR changes against base branch
• Check for test coverage
• Review code quality and security
• Provide comprehensive feedback

Initial Findings

Good News: The critical bug identified in the previous review has been fixed! ✓

The error fields in lines 693-696 now correctly reference target fields:

our: our_value.target,
proposed: value.target,

The previous bug had our_value.source and value.source instead. This is now correct.

---

View job

[petarjuki7]

makes sense, looks good to me!

[diegomrsantos]

LGTM