Spec - Share length validation (#478)

* Add share length validation in runner construction

* Align to error handling in runners constructions

* Add validation to committee runner

* Add runner construction tests

* Refactor runner construction in testingutil to deal with creation errors

* Generate JSON tests

* Fix lint issue

* Fix comments

ssv/aggregator.go

@@ -29,7 +29,12 @@ func NewAggregatorRunner(
 	operatorSigner *types.OperatorSigner,
 	valCheck qbft.ProposedValueCheckF,
 	highestDecidedSlot phase0.Slot,
-) Runner {
+) (Runner, error) {
+
+	if len(share) != 1 {
+		return nil, errors.New("must have one share")
+	}
+
 	return &AggregatorRunner{
 		BaseRunner: &BaseRunner{
 			RunnerRoleType:     types.RoleAggregator,
@@ -44,7 +49,7 @@ func NewAggregatorRunner(
 		signer:         signer,
 		operatorSigner: operatorSigner,
 		valCheck:       valCheck,
-	}
+	}, nil
 }
 
 func (r *AggregatorRunner) StartNewDuty(duty types.Duty, quorum uint64) error {

ssv/committee_runner.go

@@ -28,7 +28,10 @@ func NewCommitteeRunner(beaconNetwork types.BeaconNetwork,
 	signer types.BeaconSigner,
 	operatorSigner *types.OperatorSigner,
 	valCheck qbft.ProposedValueCheckF,
-) Runner {
+) (Runner, error) {
+	if len(share) == 0 {
+		return nil, errors.New("no shares")
+	}
 	return &CommitteeRunner{
 		BaseRunner: &BaseRunner{
 			RunnerRoleType: types.RoleCommittee,
@@ -42,7 +45,7 @@ func NewCommitteeRunner(beaconNetwork types.BeaconNetwork,
 		operatorSigner:  operatorSigner,
 		valCheck:        valCheck,
 		submittedDuties: make(map[types.BeaconRole]map[phase0.ValidatorIndex]struct{}),
-	}
+	}, nil
 }
 
 func (cr CommitteeRunner) StartNewDuty(duty types.Duty, quorum uint64) error {
@@ -261,7 +264,7 @@ func (cr CommitteeRunner) ProcessPostConsensus(signedMsg *types.PartialSignature
 	for _, att := range attestationsToSubmit {
 		attestations = append(attestations, att)
 	}
-	
+
 	if len(attestations) > 0 {
 		if err := cr.beacon.SubmitAttestations(attestations); err != nil {
 			return errors.Wrap(err, "could not submit to Beacon chain reconstructed attestation")

ssv/proposer.go

@@ -29,7 +29,12 @@ func NewProposerRunner(
 	operatorSigner *types.OperatorSigner,
 	valCheck qbft.ProposedValueCheckF,
 	highestDecidedSlot phase0.Slot,
-) Runner {
+) (Runner, error) {
+
+	if len(share) != 1 {
+		return nil, errors.New("must have one share")
+	}
+
 	return &ProposerRunner{
 		BaseRunner: &BaseRunner{
 			RunnerRoleType:     types.RoleProposer,
@@ -44,7 +49,7 @@ func NewProposerRunner(
 		signer:         signer,
 		operatorSigner: operatorSigner,
 		valCheck:       valCheck,
-	}
+	}, nil
 }
 
 func (r *ProposerRunner) StartNewDuty(duty types.Duty, quorum uint64) error {

ssv/spectest/all_tests.go

@@ -8,6 +8,7 @@ import (
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/partialsigcontainer"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/consensus"
+	runnerconstruction "github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/construction"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/duties/newduty"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/duties/proposer"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/duties/synccommitteeaggregator"
@@ -183,5 +184,9 @@ var AllTests = []tests.TestF{
 	partialsigcontainer.Quorum,
 	partialsigcontainer.Duplicate,
 	partialsigcontainer.DuplicateQuorum,
-	// partialsigcontainer.Invalid,
+	partialsigcontainer.Invalid,
+
+	runnerconstruction.OneShare,
+	runnerconstruction.NoShares,
+	runnerconstruction.ManyShares,
 }

ssv/spectest/run_test.go

@@ -20,6 +20,7 @@ import (
 	tests2 "github.com/ssvlabs/ssv-spec/ssv/spectest/tests"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/committee"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/partialsigcontainer"
+	runnerconstruction "github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/construction"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/duties/newduty"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/runner/duties/synccommitteeaggregator"
 	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests/valcheck"
@@ -172,6 +173,13 @@ func parseAndTest(t *testing.T, name string, test interface{}) {
 					Tests: typedTests,
 				}
 
+				typedTest.Run(t)
+			case reflect.TypeOf(&runnerconstruction.RunnerConstructionSpecTest{}).String():
+				byts, err := json.Marshal(test)
+				require.NoError(t, err)
+				typedTest := &runnerconstruction.RunnerConstructionSpecTest{}
+				require.NoError(t, json.Unmarshal(byts, &typedTest))
+
 				typedTest.Run(t)
 			default:
 				panic("unsupported test type " + testType)

ssv/spectest/tests/runner/construction/many_shares.go

@@ -0,0 +1,28 @@
+package runnerconstruction
+
+import (
+	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests"
+	"github.com/ssvlabs/ssv-spec/types"
+	"github.com/ssvlabs/ssv-spec/types/testingutils"
+)
+
+func ManyShares() tests.SpecTest {
+
+	ks := testingutils.KeySetMapForValidators(10)
+	shares := testingutils.ShareMapFromKeySetMap(ks)
+
+	expectedErrors := map[types.RunnerRole]string{
+		types.RoleCommittee:                 "", // No errors since it can handle multiple shares
+		types.RoleProposer:                  "must have one share",
+		types.RoleAggregator:                "must have one share",
+		types.RoleSyncCommitteeContribution: "must have one share",
+		types.RoleValidatorRegistration:     "must have one share",
+		types.RoleVoluntaryExit:             "must have one share",
+	}
+
+	return &RunnerConstructionSpecTest{
+		Name:      "many shares",
+		Shares:    shares,
+		RoleError: expectedErrors,
+	}
+}

ssv/spectest/tests/runner/construction/no_shares.go

@@ -0,0 +1,27 @@
+package runnerconstruction
+
+import (
+	"github.com/attestantio/go-eth2-client/spec/phase0"
+	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests"
+	"github.com/ssvlabs/ssv-spec/types"
+)
+
+func NoShares() tests.SpecTest {
+
+	shares := map[phase0.ValidatorIndex]*types.Share{}
+
+	expectedErrors := map[types.RunnerRole]string{
+		types.RoleCommittee:                 "no shares",
+		types.RoleProposer:                  "must have one share",
+		types.RoleAggregator:                "must have one share",
+		types.RoleSyncCommitteeContribution: "must have one share",
+		types.RoleValidatorRegistration:     "must have one share",
+		types.RoleVoluntaryExit:             "must have one share",
+	}
+
+	return &RunnerConstructionSpecTest{
+		Name:      "no shares",
+		Shares:    shares,
+		RoleError: expectedErrors,
+	}
+}

ssv/spectest/tests/runner/construction/one_share.go

@@ -0,0 +1,31 @@
+package runnerconstruction
+
+import (
+	"github.com/attestantio/go-eth2-client/spec/phase0"
+	"github.com/ssvlabs/ssv-spec/ssv/spectest/tests"
+	"github.com/ssvlabs/ssv-spec/types"
+	"github.com/ssvlabs/ssv-spec/types/testingutils"
+)
+
+func OneShare() tests.SpecTest {
+	ks := testingutils.Testing4SharesSet()
+	shares := testingutils.ShareMapFromKeySetMap(map[phase0.ValidatorIndex]*testingutils.TestKeySet{
+		testingutils.TestingValidatorIndex: ks,
+	})
+
+	// No errors since one share must be valid for all runners
+	expectedErrors := map[types.RunnerRole]string{
+		types.RoleCommittee:                 "",
+		types.RoleProposer:                  "",
+		types.RoleAggregator:                "",
+		types.RoleSyncCommitteeContribution: "",
+		types.RoleValidatorRegistration:     "",
+		types.RoleVoluntaryExit:             "",
+	}
+
+	return &RunnerConstructionSpecTest{
+		Name:      "one share",
+		Shares:    shares,
+		RoleError: expectedErrors,
+	}
+}

ssv/spectest/tests/runner/construction/test.go

@@ -0,0 +1,44 @@
+package runnerconstruction
+
+import (
+	"testing"
+
+	"github.com/attestantio/go-eth2-client/spec/phase0"
+	"github.com/ssvlabs/ssv-spec/types"
+	"github.com/ssvlabs/ssv-spec/types/testingutils"
+	"github.com/stretchr/testify/require"
+)
+
+type RunnerConstructionSpecTest struct {
+	Name      string
+	Shares    map[phase0.ValidatorIndex]*types.Share
+	RoleError map[types.RunnerRole]string
+}
+
+func (test *RunnerConstructionSpecTest) TestName() string {
+	return "RunnerConstruction " + test.Name
+}
+
+func (test *RunnerConstructionSpecTest) Run(t *testing.T) {
+
+	if len(test.RoleError) == 0 {
+		panic("no roles")
+	}
+
+	for role, expectedError := range test.RoleError {
+		// Construct runner and get construction error
+		_, err := testingutils.ConstructBaseRunnerWithShareMap(role, test.Shares)
+
+		// Check error
+		if len(expectedError) > 0 {
+			require.Error(t, err)
+			require.Contains(t, err.Error(), expectedError)
+		} else {
+			require.NoError(t, err)
+		}
+	}
+}
+
+func (test *RunnerConstructionSpecTest) GetPostState() (interface{}, error) {
+	return nil, nil
+}

ssv/sync_committee_aggregator.go

@@ -32,7 +32,12 @@ func NewSyncCommitteeAggregatorRunner(
 	operatorSigner *types.OperatorSigner,
 	valCheck qbft.ProposedValueCheckF,
 	highestDecidedSlot phase0.Slot,
-) Runner {
+) (Runner, error) {
+
+	if len(share) != 1 {
+		return nil, errors.New("must have one share")
+	}
+
 	return &SyncCommitteeAggregatorRunner{
 		BaseRunner: &BaseRunner{
 			RunnerRoleType:     types.RoleSyncCommitteeContribution,
@@ -47,7 +52,7 @@ func NewSyncCommitteeAggregatorRunner(
 		signer:         signer,
 		operatorSigner: operatorSigner,
 		valCheck:       valCheck,
-	}
+	}, nil
 }
 
 func (r *SyncCommitteeAggregatorRunner) StartNewDuty(duty types.Duty, quorum uint64) error {

ssv/validator_registration.go

@@ -27,7 +27,12 @@ func NewValidatorRegistrationRunner(
 	network Network,
 	signer types.BeaconSigner,
 	operatorSigner *types.OperatorSigner,
-) Runner {
+) (Runner, error) {
+
+	if len(share) != 1 {
+		return nil, errors.New("must have one share")
+	}
+
 	return &ValidatorRegistrationRunner{
 		BaseRunner: &BaseRunner{
 			RunnerRoleType: types.RoleValidatorRegistration,
@@ -39,7 +44,7 @@ func NewValidatorRegistrationRunner(
 		network:        network,
 		signer:         signer,
 		operatorSigner: operatorSigner,
-	}
+	}, nil
 }
 
 func (r *ValidatorRegistrationRunner) StartNewDuty(duty types.Duty, quorum uint64) error {

ssv/voluntary_exit.go

@@ -29,7 +29,12 @@ func NewVoluntaryExitRunner(
 	network Network,
 	signer types.BeaconSigner,
 	operatorSigner *types.OperatorSigner,
-) Runner {
+) (Runner, error) {
+
+	if len(share) != 1 {
+		return nil, errors.New("must have one share")
+	}
+
 	return &VoluntaryExitRunner{
 		BaseRunner: &BaseRunner{
 			RunnerRoleType: types.RoleVoluntaryExit,
@@ -41,7 +46,7 @@ func NewVoluntaryExitRunner(
 		network:        network,
 		signer:         signer,
 		operatorSigner: operatorSigner,
-	}
+	}, nil
 }
 
 func (r *VoluntaryExitRunner) StartNewDuty(duty types.Duty, quorum uint64) error {

types/testingutils/committee.go

@@ -68,7 +68,7 @@ var BaseCommitteeWithCreatorFieldsFromRunner = func(keySetMap map[phase0.Validat
 	}
 
 	createRunnerF := func(shareMap map[phase0.ValidatorIndex]*types.Share) *ssv.CommitteeRunner {
-		return ssv.NewCommitteeRunner(
+		runner, err := ssv.NewCommitteeRunner(
 			runnerSample.BaseRunner.BeaconNetwork,
 			shareMap,
 			qbft.NewController(
@@ -82,7 +82,11 @@ var BaseCommitteeWithCreatorFieldsFromRunner = func(keySetMap map[phase0.Validat
 			runnerSample.GetSigner(),
 			runnerSample.GetOperatorSigner(),
 			runnerSample.GetValCheckF(),
-		).(*ssv.CommitteeRunner)
+		)
+		if err != nil {
+			panic(err)
+		}
+		return runner.(*ssv.CommitteeRunner)
 	}
 
 	return ssv.NewCommittee(