Relax Majority Protection Rules

[GalRogozinski]

Description

We should relax majority protection rules to only checks the checkpoints epochs of a BeaconVote (attestation) against the node's own local view of the chain.

This will ensure that the DV will remain live in case of a fork. It will only fail the value check in the case the resulting SignedAttestation will make choosing the other fork a slashable offense.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR relaxes the majority fork protection rules by only validating checkpoint epochs (not roots) when checking BeaconVote attestations against the node's local chain view, allowing DVs to remain live during forks while still preventing slashable attestations.

Key changes:

• Modified BeaconVoteValueCheckF to accept phase0.Epoch parameters instead of full *phase0.Checkpoint structs
• Changed validation logic from reflect.DeepEqual (comparing both epoch and root) to simple epoch comparison
• Removed tests for unmatched source/target roots (UnmatchedSourceRoot and UnmatchedTargetRoot)
• Retained epoch mismatch tests and slashing protection tests

Issues found:

• Critical format string bug on ssv/value_check.go:60 where bv.Source (checkpoint struct) is passed to %d format specifier instead of bv.Source.Epoch

Security considerations:
The relaxed validation is appropriate because the slashing protection still validates the full AttestationData (including roots) via IsAttestationSlashable. This change only affects the pre-check against the node's expected view, not the actual slashing protection.

Confidence Score: 3/5

• Safe to merge after fixing the format string bug on line 60
• The architectural change is sound and well-tested, but there's a critical syntax error that will cause incorrect error messages (format string mismatch). The bug won't cause runtime crashes but will produce confusing error messages showing memory addresses instead of epoch numbers. All tests pass because they validate error codes, not error messages.
• ssv/value_check.go requires immediate attention to fix the format string bug on line 60

Important Files Changed

File Analysis

Filename	Score	Overview
ssv/value_check.go	3/5	Relaxed checkpoint validation to only compare epochs (not roots), but has a format string bug on line 60
ssv/spectest/all_tests.go	5/5	Removed tests for unmatched source/target roots, keeping only epoch mismatch tests
ssv/spectest/tests/valcheck/test.go	5/5	Updated test infrastructure to pass epochs instead of full checkpoints to value check function
types/testingutils/runner.go	5/5	Updated runner construction to pass checkpoint epochs instead of full checkpoint structs

Sequence Diagram

sequenceDiagram
    participant Caller
    participant BeaconVoteValueCheckF
    participant BeaconVote
    participant BeaconSigner
    
    Caller->>BeaconVoteValueCheckF: call with (signer, slot, shareKeys, expectedSourceEpoch, expectedTargetEpoch)
    BeaconVoteValueCheckF->>BeaconVote: Decode(data)
    BeaconVote-->>BeaconVoteValueCheckF: decoded BeaconVote
    
    BeaconVoteValueCheckF->>BeaconVoteValueCheckF: check source.epoch < target.epoch
    alt source >= target
        BeaconVoteValueCheckF-->>Caller: error: source >= target
    end
    
    BeaconVoteValueCheckF->>BeaconVoteValueCheckF: check source.epoch == expectedSourceEpoch
    alt source epoch mismatch
        BeaconVoteValueCheckF-->>Caller: error: checkpoint mismatch
    end
    
    BeaconVoteValueCheckF->>BeaconVoteValueCheckF: check target.epoch == expectedTargetEpoch
    alt target epoch mismatch
        BeaconVoteValueCheckF-->>Caller: error: checkpoint mismatch
    end
    
    Note over BeaconVoteValueCheckF: Root comparison removed in this PR
    
    BeaconVoteValueCheckF->>BeaconVoteValueCheckF: construct AttestationData
    
    loop for each share public key
        BeaconVoteValueCheckF->>BeaconSigner: IsAttestationSlashable(shareKey, attestationData)
        alt slashable
            BeaconSigner-->>BeaconVoteValueCheckF: error: slashable
            BeaconVoteValueCheckF-->>Caller: error: slashable
        end
        BeaconSigner-->>BeaconVoteValueCheckF: ok
    end
    
    BeaconVoteValueCheckF-->>Caller: success

Loading

[greptile-apps]

_{8 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[diegomrsantos]

I think it would help if the description explicitly stated the scope of this change (and the corresponding SIP).

With this PR, “majority fork protection” is effectively slashing protection only in majority-fork scenarios:
• BeaconVoteValueCheckF now checks only source_epoch / target_epoch against the local BN view; roots are no longer part of the value check.
• The full AttestationData (incl. roots) is used only by the per-share slashing logic, so we still avoid creating slashable attestations, but we no longer differentiate forks beyond “slashable vs non-slashable”. 

Concretely, in a Holesky-style fork:
• Some operators can be on a minority / canonical fork with different source/target roots but the same epochs as the majority fork.
• Under the new rule, they will still accept and sign an attestation built on the majority fork as long as the epochs match and it’s not slashable for their share.
• This means the DV can continue to attest on a non-canonical majority fork, even when some operators’ CLs are following the canonical minority fork.

I’d suggest adding a short “Scope and limitations” section making it explicit that:
• the mechanism guarantees slashing safety for the DV (no slashable attestations), but
• it does not guarantee staying on the canonical/minority fork in majority-client-bug events.

[GalRogozinski]

@diegomrsantos
I think I will simply make the SIP more clear a bit later and reference to it.
Changed the spec first because the tests are important for the impl.

From my perspective, this is what the SIP was always about, and we just got a bit confused :-)

[diegomrsantos]

@GalRogozinski,

I believe "Majority-Fork Slashing Protection" would be a clearer name for the SIP.

[diegomrsantos]

What do you guys think about the following idea:

Proposal: Quorum-based fork protection with a single re-sample

Motivation

• Normal 1–2 block reorgs and BN skew near the head are expected and usually resolve quickly.
• Past “strict” source+target matching across all operators hurt liveness in these benign cases, which is why it was relaxed to epoch-only checks.
• The epoch-only rule keeps liveness but effectively follows the leader’s BN view, instead of using the DV’s quorum to decide which checkpoint view to trust.

---

Rule: checkpoint quorum + one re-sample

1. First sample

   For a given attestation duty in a slot:

   • Each operator asks its BN for the attestation it would sign as a solo validator and derives a checkpoint ID:
     • checkpoint_id = (source_epoch, source_root, target_epoch, target_root).
   • The DV groups operators by checkpoint_id and counts support.

   If some checkpoint_id has ≥ signing-threshold support (e.g. 3/4):

   • Treat that as the DV’s canonical checkpoint for the slot.
   • QBFT may only decide attestation values whose (source_epoch, source_root, target_epoch, target_root) match that checkpoint_id.

2. If no checkpoint quorum → one re-sample

   If no checkpoint_id reaches signing threshold on the first sample (e.g. a 2/2 split):

   • Interpret this as “likely reorg / BN skew in progress”.
   • Wait once for a short, bounded delay inside the same slot (implementation-tuned; just enough to let a normal reorg/skew resolve).
   • Re-fetch attestation data from all operators’ BNs and recompute checkpoint_id and support.

3. Second sample decision

   • If some checkpoint_id now has ≥ signing-threshold support:
     • Treat it as canonical and run QBFT on an attestation with that checkpoint ID (with a tighter timeout).
   • If there is still no checkpoint quorum:
     • Treat this as a persistent split / serious incident and skip the attestation for this slot.
     • Emit a clear log/metric (e.g. canonical_chain_split) so operators can see this was an explicit safety choice, not a random miss.

---

🎯 Specific advantages

1. Deterministic behaviour

   • Current: which fork is attested can depend on which operator happens to be QBFT leader (their BN view).
   • Proposal: fork is determined by operator quorum on (source_epoch, source_root, target_epoch, target_root), not leader identity → deterministic and predictable for the cluster.

2. Byzantine resilience

   • Current: the leader’s BN fork choice effectively wins as long as it passes slashing + epoch checks, even if many operators are on a different fork.
   • Proposal: any attestation must use a (source_epoch, source_root, target_epoch, target_root) that has quorum support from operators’ BNs, so a single faulty leader can’t move the DV to a fork that the majority of operators don’t see.

3. Clear operational signals

   • If there is still no checkpoint quorum after the re-sample, the DV explicitly skips the attestation and logs it.
   • That’s a strong signal that operator BNs are misaligned (config, client, or network issues) or that there is an unusual fork situation, and it warrants investigation.
   • Today this kind of divergence can be masked because we just follow the leader’s view as long as basic checks pass.

4. Alignment with Ethereum’s LMD-GHOST

   • Ethereum: fork choice picks the branch with the most attestation weight, i.e. majority of validators’ latest votes.
   • Proposal: DV fork choice picks the checkpoint with quorum of operators’ BN views (internal majority).
   • Both follow the same principle: the majority view determines the canonical chain, rather than a single privileged proposer/leader.

[MatheusFranco99]

Looks good for me for now!
But really want to create a session in the future for a more robust solution.