Skip to content

always blind blocks - #608

Open
nkryuchkov wants to merge 11 commits into
mainfrom
proposer-blinded-qbft
Open

always blind blocks#608
nkryuchkov wants to merge 11 commits into
mainfrom
proposer-blinded-qbft

Conversation

@nkryuchkov

Copy link
Copy Markdown
Contributor

Summary

This PR updates ssv-spec proposer flow so QBFT always agrees on the blinded block form.

What Changed

  • Updated proposer pre-consensus flow to blind fetched beacon blocks before encoding ValidatorConsensusData
  • Regenerated affected proposer spectest JSON and state-comparison fixtures

Why

The spec should define the consensus algorithm, and the important invariant here is that proposer QBFT values are blinded. This reduces the mismatch between ssv-spec and ssv while keeping the spec focused on protocol behavior rather than node-specific mechanics.

@greptile-apps

greptile-apps Bot commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR enforces that QBFT consensus for proposer duties always operates on the blinded block form, regardless of whether the beacon node returned a full or blinded block. A new ensureBlindedProposal function in ssv/proposer.go derives the blinded form locally when needed, covering all four forks (Capella, Deneb, Electra, Fulu). The ProposerCalls interface is simultaneously simplified by dropping the dead ssz.Marshaler second return value from GetBeaconBlock.

Key changes:

  • ensureBlindedProposal correctly handles all 8 paths (4 forks × already-blinded vs. full block), with proper nil guards, slices.Clone for slice fields, and new(uint256.Int).Set(...) for the pointer-typed BaseFeePerGas on Deneb+
  • Two tests that represented now-impossible scenarios (ProposeBlindedBlockDecidedRegular, BlindedRunnerAcceptsNormalBlock) are cleanly removed; the corresponding JSON fixtures and state-comparison helpers are removed as well
  • proposer_blinding_unit_test.go is added with direct unit coverage of all 8 ensureBlindedProposal paths, complementing the regenerated integration-level spec-test fixtures
  • TestingBlindedBeaconBlock* constructors in testingutils are updated to use deep copies consistently, aligning with the production blinding logic

Confidence Score: 5/5

  • PR is safe to merge; the logic is sound, all 8 code paths are covered by new unit tests, and no critical issues were found.
  • The change is well-scoped and well-tested: a new unit test file directly covers every branch of ensureBlindedProposal, the integration fixtures are regenerated consistently, and the two removed tests (ProposeBlindedBlockDecidedRegular, BlindedRunnerAcceptsNormalBlock) correctly represent now-unreachable scenarios. The interface simplification in ssv/types.go is clean. No logic errors, missing nil checks, or field-mapping gaps were identified.
  • No files require special attention.

Important Files Changed

Filename Overview
ssv/proposer.go New ensureBlindedProposal function correctly converts full block proposals to blinded form for all 4 supported forks (Capella, Deneb, Electra, Fulu); ProcessPreConsensus updated to always call it. Logic is sound and nil-guarded.
ssv/types.go Interface GetBeaconBlock simplified by removing the dead ssz.Marshaler return value; comment added to SubmitBeaconBlock clarifying responsibility for blob reconstruction.
ssv/proposer_blinding_unit_test.go New unit test file covering all 8 code paths in ensureBlindedProposal (4 forks × already-blinded / full block). Uses apiv1fulu.BlockContents correctly for the Fulu case.
types/testingutils/beacon_node.go GetBeaconBlock simplified to return (*api.VersionedProposal, error) without the SSZ marshaler; always returns a full block since blinding is now done by the runner.
types/testingutils/beacon_node_proposer.go All Testing*BlindedBeaconBlock* constructors updated to use slices.Clone for slice fields and new(uint256.Int).Set(...) for BaseFeePerGas (Deneb+), preventing accidental aliasing.
ssv/spectest/tests/runner/duties/proposer/blinded_sc.go Removed the separate fullHappyFlowBlindedProposerReceivingNormalBlockSC helper; fullHappyFlowProposerReceivingBlindedBlockSC now correctly uses ProposerRunner (not the blinded-runner variant) since both produce blinded QBFT values.
ssv/spectest/all_tests.go Removed ProposeBlindedBlockDecidedRegular and BlindedRunnerAcceptsNormalBlock tests, which are no longer valid distinct scenarios since consensus always operates on blinded blocks.

Sequence Diagram

sequenceDiagram
    participant R as ProposerRunner
    participant BN as BeaconNode
    participant EB as ensureBlindedProposal
    participant QBFT as QBFT Consensus

    R->>BN: GetBeaconBlock(slot, graffiti, randao)
    BN-->>R: *VersionedProposal (may be full or blinded)
    R->>EB: ensureBlindedProposal(vBlk)
    alt already blinded
        EB-->>R: (p, blindedObj, nil) unchanged
    else full block (Capella/Deneb/Electra/Fulu)
        EB->>EB: compute txRoot + withdrawalsRoot
        EB->>EB: construct BlindedBeaconBlock
        EB-->>R: (blindedVBlk, blindedObj, nil)
    end
    R->>R: blindedObj.MarshalSSZ() → byts
    R->>QBFT: Start(ValidatorConsensusData{blinded byts})
    QBFT-->>R: DecidedValue (always blinded form)
    R->>BN: SubmitBeaconBlock(decided blinded block, sig)
Loading

Reviews (9): Last reviewed commit: "code review comments" | Re-trigger Greptile

Comment thread ssv/proposer.go Outdated
Comment thread ssv/proposer.go
@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@nkryuchkov

Copy link
Copy Markdown
Contributor Author

@greptileai please review it again

@y0sher y0sher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm code wise,but @GalRogozinski should probably approve if he wants this change in the spec.

@GalRogozinski GalRogozinski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

denied until discussion concludes:
ssvlabs/SIPs#78 (reply in thread)

@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale due to 60 days of inactivity.

@github-actions github-actions Bot added the stale label May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants