protocol: majority fork protection (#2358)

* majority fork protection

* fix linter

* fix log

* fix returned value

* fix log text

* fix checkConsensusDataSameToOwn logic for aggregator

* only check source/target attestation epoch

* value checker interface

* create a type for SlashingProtectionData

* rename ValueCheckF

* add a unit test

* update comment about spec decision

* spec alignment

* code review

* differ

* differ

* differ

* fix build issue

* differ

* fix issues after merging

* check target root (#2545)

* 2358: iurii refactor (#2551)

* 2358: iurii refactor

* clarify what valCheck is

* fix linter

* fix spec tests

* cleanup

* fix marshaling spectest

* cleanup ValueChecker marshaling leftovers

---------

Co-authored-by: Nikita Kryuchkov <[email protected]>

* code review comments

* differ

* update checks as per SIP

---------

Co-authored-by: iurii-ssv <[email protected]>
Co-authored-by: rehs0y <[email protected]>

Author: 3 people

operator/validator/controller.go

@@ -1030,11 +1030,10 @@ func SetupCommitteeRunners(
 	ctx context.Context,
 	options *validator.Options,
 ) validator.CommitteeRunnerFunc {
-	buildController := func(role spectypes.RunnerRole, valueCheckF specqbft.ProposedValueCheckF) *qbftcontroller.Controller {
+	buildController := func(role spectypes.RunnerRole) *qbftcontroller.Controller {
 		config := &qbft.Config{
 			BeaconSigner: options.Signer,
 			Domain:       options.NetworkConfig.DomainType,
-			ValueCheckF:  valueCheckF,
 			ProposerF: func(state *specqbft.State, round specqbft.Round) spectypes.OperatorID {
 				leader := qbft.RoundRobinProposer(state, round)
 				return leader
@@ -1055,18 +1054,15 @@ func SetupCommitteeRunners(
 		attestingValidators []phase0.BLSPubKey,
 		dutyGuard runner.CommitteeDutyGuard,
 	) (*runner.CommitteeRunner, error) {
-		// Create a committee runner.
-		epoch := options.NetworkConfig.EstimatedEpochAtSlot(slot)
-		valCheck := ssv.BeaconVoteValueCheckF(options.Signer, slot, attestingValidators, epoch)
 		crunner, err := runner.NewCommitteeRunner(
 			options.NetworkConfig,
 			shares,
-			buildController(spectypes.RoleCommittee, valCheck),
+			attestingValidators,
+			buildController(spectypes.RoleCommittee),
 			options.Beacon,
 			options.Network,
 			options.Signer,
 			options.OperatorSigner,
-			valCheck,
 			dutyGuard,
 			options.DoppelgangerHandler,
 		)
@@ -1095,11 +1091,10 @@ func SetupRunners(
 		spectypes.RoleVoluntaryExit,
 	}
 
-	buildController := func(role spectypes.RunnerRole, valueCheckF specqbft.ProposedValueCheckF) *qbftcontroller.Controller {
+	buildController := func(role spectypes.RunnerRole) *qbftcontroller.Controller {
 		config := &qbft.Config{
 			BeaconSigner: options.Signer,
 			Domain:       options.NetworkConfig.DomainType,
-			ValueCheckF:  nil, // is set per role type
 			ProposerF: func(state *specqbft.State, round specqbft.Round) spectypes.OperatorID {
 				leader := qbft.RoundRobinProposer(state, round)
 				return leader
@@ -1108,7 +1103,6 @@ func SetupRunners(
 			Timer:       roundtimer.New(ctx, options.NetworkConfig.Beacon, role, nil),
 			CutOffRound: roundtimer.CutOffRound,
 		}
-		config.ValueCheckF = valueCheckF
 
 		identifier := spectypes.NewMsgID(options.NetworkConfig.DomainType, share.ValidatorPubKey[:], role)
 		qbftCtrl := qbftcontroller.NewController(identifier[:], operator, config, options.OperatorSigner, options.FullNode)
@@ -1123,17 +1117,17 @@ func SetupRunners(
 	for _, role := range runnersType {
 		switch role {
 		case spectypes.RoleProposer:
-			proposedValueCheck := ssv.ProposerValueCheckF(options.Signer, options.NetworkConfig.Beacon, share.ValidatorPubKey, share.ValidatorIndex, phase0.BLSPubKey(share.SharePubKey))
-			qbftCtrl := buildController(spectypes.RoleProposer, proposedValueCheck)
+			proposedValueCheck := ssv.NewProposerChecker(options.Signer, options.NetworkConfig.Beacon, share.ValidatorPubKey, share.ValidatorIndex, phase0.BLSPubKey(share.SharePubKey))
+			qbftCtrl := buildController(spectypes.RoleProposer)
 			runners[role], err = runner.NewProposerRunner(logger, options.NetworkConfig, shareMap, qbftCtrl, options.Beacon, options.Network, options.Signer, options.OperatorSigner, options.DoppelgangerHandler, proposedValueCheck, 0, options.Graffiti, options.ProposerDelay)
 		case spectypes.RoleAggregator:
-			aggregatorValueCheckF := ssv.AggregatorValueCheckF(options.Signer, options.NetworkConfig.Beacon, share.ValidatorPubKey, share.ValidatorIndex)
-			qbftCtrl := buildController(spectypes.RoleAggregator, aggregatorValueCheckF)
-			runners[role], err = runner.NewAggregatorRunner(options.NetworkConfig, shareMap, qbftCtrl, options.Beacon, options.Network, options.Signer, options.OperatorSigner, aggregatorValueCheckF, 0)
+			aggregatorValueChecker := ssv.NewAggregatorChecker(options.NetworkConfig.Beacon, share.ValidatorPubKey, share.ValidatorIndex)
+			qbftCtrl := buildController(spectypes.RoleAggregator)
+			runners[role], err = runner.NewAggregatorRunner(options.NetworkConfig, shareMap, qbftCtrl, options.Beacon, options.Network, options.Signer, options.OperatorSigner, aggregatorValueChecker, 0)
 		case spectypes.RoleSyncCommitteeContribution:
-			syncCommitteeContributionValueCheckF := ssv.SyncCommitteeContributionValueCheckF(options.Signer, options.NetworkConfig.Beacon, share.ValidatorPubKey, share.ValidatorIndex)
-			qbftCtrl := buildController(spectypes.RoleSyncCommitteeContribution, syncCommitteeContributionValueCheckF)
-			runners[role], err = runner.NewSyncCommitteeAggregatorRunner(options.NetworkConfig, shareMap, qbftCtrl, options.Beacon, options.Network, options.Signer, options.OperatorSigner, syncCommitteeContributionValueCheckF, 0)
+			syncCommitteeContributionValueChecker := ssv.NewSyncCommitteeContributionChecker(options.NetworkConfig.Beacon, share.ValidatorPubKey, share.ValidatorIndex)
+			qbftCtrl := buildController(spectypes.RoleSyncCommitteeContribution)
+			runners[role], err = runner.NewSyncCommitteeAggregatorRunner(options.NetworkConfig, shareMap, qbftCtrl, options.Beacon, options.Network, options.Signer, options.OperatorSigner, syncCommitteeContributionValueChecker, 0)
 		case spectypes.RoleValidatorRegistration:
 			runners[role], err = runner.NewValidatorRegistrationRunner(options.NetworkConfig, shareMap, options.Beacon, options.Network, options.Signer, options.OperatorSigner, validatorRegistrationSubmitter, validatorStore, options.GasLimit)
 		case spectypes.RoleVoluntaryExit:

protocol/v2/qbft/config.go

@@ -4,11 +4,10 @@ import (
 	specqbft "github.com/ssvlabs/ssv-spec/qbft"
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 
-	"github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer"
 	"github.com/ssvlabs/ssv/ssvsigner/ekm"
-)
 
-var CutOffRound specqbft.Round = specqbft.Round(specqbft.CutoffRound)
+	"github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer"
+)
 
 type signing interface {
 	// GetShareSigner returns a BeaconSigner instance
@@ -19,8 +18,6 @@ type signing interface {
 
 type IConfig interface {
 	signing
-	// GetValueCheckF returns value check function
-	GetValueCheckF() specqbft.ProposedValueCheckF
 	// GetProposerF returns func used to calculate proposer
 	GetProposerF() specqbft.ProposerF
 	// GetNetwork returns a p2p Network instance
@@ -34,7 +31,6 @@ type IConfig interface {
 type Config struct {
 	BeaconSigner ekm.BeaconSigner
 	Domain       spectypes.DomainType
-	ValueCheckF  specqbft.ProposedValueCheckF
 	ProposerF    specqbft.ProposerF
 	Network      specqbft.Network
 	Timer        roundtimer.Timer
@@ -51,11 +47,6 @@ func (c *Config) GetSignatureDomainType() spectypes.DomainType {
 	return c.Domain
 }
 
-// GetValueCheckF returns value check instance
-func (c *Config) GetValueCheckF() specqbft.ProposedValueCheckF {
-	return c.ValueCheckF
-}
-
 // GetProposerF returns func used to calculate proposer
 func (c *Config) GetProposerF() specqbft.ProposerF {
 	return c.ProposerF

protocol/v2/qbft/controller/controller.go

@@ -21,6 +21,7 @@ import (
 	"github.com/ssvlabs/ssv/protocol/v2/qbft"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/instance"
 	qbftstorage "github.com/ssvlabs/ssv/protocol/v2/qbft/storage"
+	"github.com/ssvlabs/ssv/protocol/v2/ssv"
 	ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types"
 )
 
@@ -60,13 +61,19 @@ func NewController(
 }
 
 // StartNewInstance will start a new QBFT instance, if can't will return error
-func (c *Controller) StartNewInstance(ctx context.Context, logger *zap.Logger, height specqbft.Height, value []byte) error {
+func (c *Controller) StartNewInstance(
+	ctx context.Context,
+	logger *zap.Logger,
+	height specqbft.Height,
+	value []byte,
+	valueChecker ssv.ValueChecker,
+) error {
 	ctx, span := tracer.Start(ctx,
 		observability.InstrumentName(observabilityNamespace, "qbft.controller.start"),
 		trace.WithAttributes(observability.BeaconSlotAttribute(phase0.Slot(height))))
 	defer span.End()
 
-	if err := c.GetConfig().GetValueCheckF()(value); err != nil {
+	if err := valueChecker.CheckValue(value); err != nil {
 		return traces.Errorf(span, "value invalid: %w", err)
 	}
 
@@ -83,7 +90,7 @@ func (c *Controller) StartNewInstance(ctx context.Context, logger *zap.Logger, h
 	newInstance := c.addAndStoreNewInstance()
 
 	span.AddEvent("start new instance")
-	newInstance.Start(ctx, logger, value, height)
+	newInstance.Start(ctx, logger, value, height, valueChecker)
 	c.forceStopAllInstanceExceptCurrent()
 
 	span.SetStatus(codes.Ok, "")

protocol/v2/qbft/instance/instance.go

@@ -18,6 +18,7 @@ import (
 	"github.com/ssvlabs/ssv/observability"
 	"github.com/ssvlabs/ssv/observability/log/fields"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft"
+	"github.com/ssvlabs/ssv/protocol/v2/ssv"
 	ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types"
 )
 
@@ -31,8 +32,9 @@ type Instance struct {
 	processMsgF *spectypes.ThreadSafeF
 	startOnce   sync.Once
 
-	forceStop  bool
-	StartValue []byte
+	forceStop    bool
+	StartValue   []byte
+	ValueChecker ssv.ValueChecker `json:"-"`
 
 	metrics *metricsRecorder
 }
@@ -73,7 +75,13 @@ func (i *Instance) ForceStop() {
 }
 
 // Start is an interface implementation
-func (i *Instance) Start(ctx context.Context, logger *zap.Logger, value []byte, height specqbft.Height) {
+func (i *Instance) Start(
+	ctx context.Context,
+	logger *zap.Logger,
+	value []byte,
+	height specqbft.Height,
+	valueChecker ssv.ValueChecker,
+) {
 	i.startOnce.Do(func() {
 		_, span := tracer.Start(ctx,
 			observability.InstrumentName(observabilityNamespace, "qbft.instance.start"),
@@ -83,6 +91,7 @@ func (i *Instance) Start(ctx context.Context, logger *zap.Logger, value []byte,
 		i.StartValue = value
 		i.bumpToRound(specqbft.FirstRound)
 		i.State.Height = height
+		i.ValueChecker = valueChecker
 		i.metrics.Start()
 		i.config.GetTimer().TimeoutForRound(height, specqbft.FirstRound)
 

protocol/v2/qbft/instance/proposal.go

@@ -137,7 +137,7 @@ func (i *Instance) isProposalJustification(
 	round specqbft.Round,
 	fullData []byte,
 ) error {
-	if err := i.config.GetValueCheckF()(fullData); err != nil {
+	if err := i.ValueChecker.CheckValue(fullData); err != nil {
 		return errors.Wrap(err, "proposal fullData invalid")
 	}
 

protocol/v2/qbft/spectest/controller_type.go

@@ -145,8 +145,14 @@ func testBroadcastedDecided(
 	}
 }
 
-func runInstanceWithData(t *testing.T, logger *zap.Logger, height specqbft.Height, contr *controller.Controller, runData *spectests.RunInstanceData) error {
-	err := contr.StartNewInstance(context.TODO(), logger, height, runData.InputValue)
+func runInstanceWithData(
+	t *testing.T,
+	logger *zap.Logger,
+	height specqbft.Height,
+	contr *controller.Controller,
+	runData *spectests.RunInstanceData,
+) error {
+	err := contr.StartNewInstance(context.TODO(), logger, height, runData.InputValue, qbfttesting.TestingValueChecker{})
 	var lastErr error
 	if err != nil {
 		lastErr = err

protocol/v2/qbft/spectest/msg_processing_type.go

@@ -39,6 +39,7 @@ func RunMsgProcessing(t *testing.T, test *spectests.MsgProcessingSpecTest) {
 		test.Pre.State.Height,
 		signer,
 	)
+	pre.ValueChecker = qbfttesting.TestingValueChecker{}
 	require.NoError(t, pre.Decode(preByts))
 
 	preInstance := pre

protocol/v2/qbft/testing/utils.go

@@ -9,27 +9,31 @@ import (
 	"github.com/ssvlabs/ssv-spec/types/testingutils"
 	"go.uber.org/zap"
 
+	"github.com/ssvlabs/ssv/ssvsigner/ekm"
+
 	"github.com/ssvlabs/ssv/protocol/v2/qbft"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/controller"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer"
-	"github.com/ssvlabs/ssv/ssvsigner/ekm"
 )
 
+type TestingValueChecker struct{}
+
+func (TestingValueChecker) CheckValue(data []byte) error {
+	if bytes.Equal(data, TestingInvalidValueCheck) {
+		return errors.New("invalid value")
+	}
+
+	// as a base validation we do not accept nil values
+	if len(data) == 0 {
+		return errors.New("invalid value")
+	}
+	return nil
+}
+
 var TestingConfig = func(logger *zap.Logger, keySet *testingutils.TestKeySet) *qbft.Config {
 	return &qbft.Config{
 		BeaconSigner: ekm.NewTestingKeyManagerAdapter(testingutils.NewTestingKeyManager()),
 		Domain:       testingutils.TestingSSVDomainType,
-		ValueCheckF: func(data []byte) error {
-			if bytes.Equal(data, TestingInvalidValueCheck) {
-				return errors.New("invalid value")
-			}
-
-			// as a base validation we do not accept nil values
-			if len(data) == 0 {
-				return errors.New("invalid value")
-			}
-			return nil
-		},
 		ProposerF: func(state *specqbft.State, round specqbft.Round) types.OperatorID {
 			return 1
 		},

protocol/v2/ssv/runner/aggregator.go

@@ -28,6 +28,7 @@ import (
 	"github.com/ssvlabs/ssv/observability/traces"
 	"github.com/ssvlabs/ssv/protocol/v2/blockchain/beacon"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/controller"
+	"github.com/ssvlabs/ssv/protocol/v2/ssv"
 	ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types"
 )
 
@@ -38,9 +39,11 @@ type AggregatorRunner struct {
 	network        specqbft.Network
 	signer         ekm.BeaconSigner
 	operatorSigner ssvtypes.OperatorSigner
-	valCheck       specqbft.ProposedValueCheckF
 	measurements   measurementsStore
 
+	// ValCheck is used to validate the qbft-value(s) proposed by other Operators.
+	ValCheck ssv.ValueChecker
+
 	// IsAggregator returns true if the signature is from the input validator. The committee
 	// count is provided as an argument rather than imported implementation from spec. Having
 	// committee count as an argument allows cheaper computation at run time.
@@ -66,7 +69,7 @@ func NewAggregatorRunner(
 	network specqbft.Network,
 	signer ekm.BeaconSigner,
 	operatorSigner ssvtypes.OperatorSigner,
-	valCheck specqbft.ProposedValueCheckF,
+	valCheck ssv.ValueChecker,
 	highestDecidedSlot phase0.Slot,
 ) (*AggregatorRunner, error) {
 	if len(share) != 1 {
@@ -86,7 +89,7 @@ func NewAggregatorRunner(
 		network:        network,
 		signer:         signer,
 		operatorSigner: operatorSigner,
-		valCheck:       valCheck,
+		ValCheck:       valCheck,
 		measurements:   NewMeasurementsStore(),
 
 		IsAggregator: isAggregatorFn(),
@@ -185,7 +188,7 @@ func (r *AggregatorRunner) ProcessPreConsensus(ctx context.Context, logger *zap.
 		DataSSZ: byts,
 	}
 
-	if err := r.BaseRunner.decide(ctx, logger, r, duty.Slot, input); err != nil {
+	if err := r.BaseRunner.decide(ctx, logger, r, duty.Slot, input, r.ValCheck); err != nil {
 		return traces.Errorf(span, "can't start new duty runner instance for duty: %w", err)
 	}
 
@@ -204,7 +207,7 @@ func (r *AggregatorRunner) ProcessConsensus(ctx context.Context, logger *zap.Log
 	defer span.End()
 
 	span.AddEvent("checking if instance is decided")
-	decided, encDecidedValue, err := r.BaseRunner.baseConsensusMsgProcessing(ctx, logger, r.GetValCheckF(), signedMsg, &spectypes.ValidatorConsensusData{})
+	decided, encDecidedValue, err := r.BaseRunner.baseConsensusMsgProcessing(ctx, logger, r.ValCheck.CheckValue, signedMsg, &spectypes.ValidatorConsensusData{})
 	if err != nil {
 		return traces.Errorf(span, "failed processing consensus message: %w", err)
 	}
@@ -487,10 +490,6 @@ func (r *AggregatorRunner) GetState() *State {
 	return r.BaseRunner.State
 }
 
-func (r *AggregatorRunner) GetValCheckF() specqbft.ProposedValueCheckF {
-	return r.valCheck
-}
-
 func (r *AggregatorRunner) GetSigner() ekm.BeaconSigner {
 	return r.signer
 }