cmd: move threshold check to CLI level

cmd/createcluster.go

@@ -10,7 +10,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"math"
 	"net/url"
 	"os"
 	"path"
@@ -100,6 +99,22 @@ func newCreateClusterCmd(runFunc func(context.Context, io.Writer, clusterConfig)
 	bindClusterFlags(cmd.Flags(), &conf)
 	bindInsecureFlags(cmd.Flags(), &conf.InsecureKeys)
 
+	wrapPreRunE(cmd, func(cmd *cobra.Command, _ []string) error {
+		thresholdPresent := cmd.Flags().Lookup("threshold").Changed
+
+		if thresholdPresent {
+			if conf.Threshold < minThreshold {
+				return errors.New("threshold must be greater than 1", z.Int("threshold", conf.Threshold), z.Int("min", minThreshold))
+			}
+			if conf.Threshold > conf.NumNodes {
+				return errors.New("threshold cannot be greater than number of operators",
+					z.Int("threshold", conf.Threshold), z.Int("operators", conf.NumNodes))
+			}
+		}
+
+		return nil
+	})
+
 	return cmd
 }
 
@@ -374,16 +389,6 @@ func validateCreateConfig(ctx context.Context, conf clusterConfig) error {
 		return errors.New("number of operators is below minimum", z.Int("operators", conf.NumNodes), z.Int("min", minNodes))
 	}
 
-	// Check for threshold parameter
-	minThreshold := int(math.Ceil(float64(conf.NumNodes*2) / 3))
-	if conf.Threshold < minThreshold {
-		return errors.New("threshold cannot be smaller than BFT quorum", z.Int("threshold", conf.Threshold), z.Int("min", minThreshold))
-	}
-	if conf.Threshold > conf.NumNodes {
-		return errors.New("threshold cannot be greater than number of operators",
-			z.Int("threshold", conf.Threshold), z.Int("operators", conf.NumNodes))
-	}
-
 	return nil
 }
 

cmd/createcluster_internal_test.go

@@ -250,26 +250,6 @@ func TestCreateCluster(t *testing.T) {
 				},
 			},
 		},
-		{
-			Name: "threshold greater than the number of operators",
-			Config: clusterConfig{
-				NumNodes:  4,
-				Threshold: 5,
-				NumDVs:    1,
-				Network:   defaultNetwork,
-			},
-			expectedErr: "threshold cannot be greater than number of operators",
-		},
-		{
-			Name: "threshold smaller than BFT quorum",
-			Config: clusterConfig{
-				NumNodes:  4,
-				Threshold: 2,
-				NumDVs:    1,
-				Network:   defaultNetwork,
-			},
-			expectedErr: "threshold cannot be smaller than BFT quorum",
-		},
 		{
 			Name: "test with number of nodes below minimum",
 			Config: clusterConfig{
@@ -788,6 +768,82 @@ func TestPublish(t *testing.T) {
 	})
 }
 
+func TestClusterCLI(t *testing.T) {
+	feeRecipientArg := "--fee-recipient-addresses=" + validEthAddr
+	withdrawalArg := "--withdrawal-addresses=" + validEthAddr
+
+	tests := []struct {
+		name          string
+		network       string
+		nodes         string
+		numValidators string
+		feeRecipient  string
+		withdrawal    string
+		threshold     string
+		expectedErr   string
+		cleanup       func(*testing.T)
+	}{
+		{
+			name:          "threshold below minimum",
+			nodes:         "--nodes=3",
+			network:       "--network=holesky",
+			numValidators: "--num-validators=1",
+			feeRecipient:  feeRecipientArg,
+			withdrawal:    withdrawalArg,
+			threshold:     "--threshold=1",
+			expectedErr:   "threshold must be greater than 1",
+		},
+		{
+			name:          "threshold above maximum",
+			nodes:         "--nodes=4",
+			network:       "--network=holesky",
+			numValidators: "--num-validators=1",
+			feeRecipient:  feeRecipientArg,
+			withdrawal:    withdrawalArg,
+			threshold:     "--threshold=5",
+			expectedErr:   "threshold cannot be greater than number of operators",
+		},
+		{
+			name:          "no threshold provided",
+			nodes:         "--nodes=3",
+			network:       "--network=holesky",
+			numValidators: "--num-validators=1",
+			feeRecipient:  feeRecipientArg,
+			withdrawal:    withdrawalArg,
+			threshold:     "",
+			expectedErr:   "",
+			cleanup: func(t *testing.T) {
+				t.Helper()
+				require.NoError(t, os.RemoveAll("node0"))
+				require.NoError(t, os.RemoveAll("node1"))
+				require.NoError(t, os.RemoveAll("node2"))
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cmd := newCreateCmd(newCreateClusterCmd(runCreateCluster))
+			if test.threshold != "" {
+				cmd.SetArgs([]string{"cluster", test.nodes, test.feeRecipient, test.withdrawal, test.network, test.numValidators, test.threshold})
+			} else {
+				cmd.SetArgs([]string{"cluster", test.nodes, test.feeRecipient, test.withdrawal, test.network, test.numValidators})
+			}
+
+			err := cmd.Execute()
+			if test.expectedErr != "" {
+				require.ErrorContains(t, err, test.expectedErr)
+			} else {
+				require.NoError(t, err)
+			}
+
+			if test.cleanup != nil {
+				test.cleanup(t)
+			}
+		})
+	}
+}
+
 // mockKeymanagerReq is a mock keymanager request for use in tests.
 type mockKeymanagerReq struct {
 	Keystores []string `json:"keystores"`

cmd/createdkg.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	crand "crypto/rand"
 	"encoding/json"
-	"math"
 	"os"
 	"path"
 
@@ -50,6 +49,22 @@ func newCreateDKGCmd(runFunc func(context.Context, createDKGConfig) error) *cobr
 
 	bindCreateDKGFlags(cmd, &config)
 
+	wrapPreRunE(cmd, func(cmd *cobra.Command, _ []string) error {
+		thresholdPresent := cmd.Flags().Lookup("threshold").Changed
+
+		if thresholdPresent {
+			if config.Threshold < minThreshold {
+				return errors.New("threshold must be greater than 1", z.Int("threshold", config.Threshold), z.Int("min", minThreshold))
+			}
+			if config.Threshold > len(config.OperatorENRs) {
+				return errors.New("threshold cannot be greater than number of operators",
+					z.Int("threshold", config.Threshold), z.Int("operators", len(config.OperatorENRs)))
+			}
+		}
+
+		return nil
+	})
+
 	return cmd
 }
 
@@ -82,7 +97,7 @@ func runCreateDKG(ctx context.Context, conf createDKGConfig) (err error) {
 		conf.Network = eth2util.Goerli.Name
 	}
 
-	if err = validateDKGConfig(conf.Threshold, len(conf.OperatorENRs), conf.Network, conf.DepositAmounts); err != nil {
+	if err = validateDKGConfig(len(conf.OperatorENRs), conf.Network, conf.DepositAmounts); err != nil {
 		return err
 	}
 
@@ -115,7 +130,7 @@ func runCreateDKG(ctx context.Context, conf createDKGConfig) (err error) {
 	safeThreshold := cluster.Threshold(len(conf.OperatorENRs))
 	if conf.Threshold == 0 {
 		conf.Threshold = safeThreshold
-	} else if conf.Threshold != safeThreshold {
+	} else {
 		log.Warn(ctx, "Non standard `--threshold` flag provided, this will affect cluster safety", nil, z.Int("threshold", conf.Threshold), z.Int("safe_threshold", safeThreshold))
 	}
 
@@ -181,22 +196,12 @@ func validateWithdrawalAddrs(addrs []string, network string) error {
 }
 
 // validateDKGConfig returns an error if any of the provided config parameter is invalid.
-func validateDKGConfig(threshold, numOperators int, network string, depositAmounts []int) error {
+func validateDKGConfig(numOperators int, network string, depositAmounts []int) error {
 	// Don't allow cluster size to be less than 3.
 	if numOperators < minNodes {
 		return errors.New("number of operators is below minimum", z.Int("operators", numOperators), z.Int("min", minNodes))
 	}
 
-	// Ensure threshold setting is sound
-	minThreshold := int(math.Ceil(float64(numOperators*2) / 3))
-	if threshold < minThreshold {
-		return errors.New("threshold cannot be smaller than BFT quorum", z.Int("threshold", threshold), z.Int("min", minThreshold))
-	}
-	if threshold > numOperators {
-		return errors.New("threshold cannot be greater than length of operators",
-			z.Int("threshold", threshold), z.Int("operators", numOperators))
-	}
-
 	if !eth2util.ValidNetwork(network) {
 		return errors.New("unsupported network", z.Str("network", network))
 	}

cmd/createdkg_internal_test.go

@@ -184,36 +184,109 @@ func TestValidateWithdrawalAddr(t *testing.T) {
 }
 
 func TestValidateDKGConfig(t *testing.T) {
-	t.Run("threshold exceeds numOperators", func(t *testing.T) {
-		threshold := 5
-		numOperators := 4
-		err := validateDKGConfig(threshold, numOperators, "", nil)
-		require.ErrorContains(t, err, "threshold cannot be greater than length of operators")
-	})
-
-	t.Run("threshold equals 1", func(t *testing.T) {
-		threshold := 1
-		numOperators := 3
-		err := validateDKGConfig(threshold, numOperators, "", nil)
-		require.ErrorContains(t, err, "threshold cannot be smaller than BFT quorum")
-	})
 
 	t.Run("insufficient ENRs", func(t *testing.T) {
-		threshold := 2
 		numOperators := 2
-		err := validateDKGConfig(threshold, numOperators, "", nil)
+		err := validateDKGConfig(numOperators, "", nil)
 		require.ErrorContains(t, err, "number of operators is below minimum")
 	})
 
 	t.Run("invalid network", func(t *testing.T) {
-		threshold := 3
 		numOperators := 4
-		err := validateDKGConfig(threshold, numOperators, "cosmos", nil)
+		err := validateDKGConfig(numOperators, "cosmos", nil)
 		require.ErrorContains(t, err, "unsupported network")
 	})
 
 	t.Run("wrong deposit amounts sum", func(t *testing.T) {
-		err := validateDKGConfig(3, 4, "goerli", []int{8, 16})
+		err := validateDKGConfig(4, "goerli", []int{8, 16})
 		require.ErrorContains(t, err, "sum of partial deposit amounts must sum up to 32ETH")
 	})
 }
+
+func TestDKGCLI(t *testing.T) {
+	var enrs []string
+	for range minNodes {
+		enrs = append(enrs, "enr:-JG4QG472ZVvl8ySSnUK9uNVDrP_hjkUrUqIxUC75aayzmDVQedXkjbqc7QKyOOS71VmlqnYzri_taV8ZesFYaoQSIOGAYHtv1WsgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKwwq_CAld6oVKOrixE-JzMtvvNgb9yyI-_rwq4NFtajIN0Y3CCDhqDdWRwgg4u")
+	}
+	enrArg := "--operator-enrs=" + strings.Join(enrs, ",")
+	feeRecipientArg := "--fee-recipient-addresses=" + validEthAddr
+	withdrawalArg := "--withdrawal-addresses=" + validEthAddr
+	outputDirArg := "--output-dir=.charon"
+
+	tests := []struct {
+		name         string
+		enr          string
+		feeRecipient string
+		withdrawal   string
+		outputDir    string
+		threshold    string
+		expectedErr  string
+		prepare      func(*testing.T)
+		cleanup      func(*testing.T)
+	}{
+		{
+			name:         "threshold below minimum",
+			enr:          enrArg,
+			feeRecipient: feeRecipientArg,
+			withdrawal:   withdrawalArg,
+			outputDir:    outputDirArg,
+			threshold:    "--threshold=1",
+			expectedErr:  "threshold must be greater than 1",
+		},
+		{
+			name:         "threshold above maximum",
+			enr:          enrArg,
+			feeRecipient: feeRecipientArg,
+			withdrawal:   withdrawalArg,
+			outputDir:    outputDirArg,
+			threshold:    "--threshold=4",
+			expectedErr:  "threshold cannot be greater than number of operators",
+		},
+		{
+			name:         "no threshold provided",
+			enr:          enrArg,
+			feeRecipient: feeRecipientArg,
+			withdrawal:   withdrawalArg,
+			outputDir:    outputDirArg,
+			threshold:    "",
+			expectedErr:  "",
+			prepare: func(t *testing.T) {
+				t.Helper()
+				charonDir := testutil.CreateTempCharonDir(t)
+				b := []byte("sample definition")
+				require.NoError(t, os.WriteFile(path.Join(charonDir, "cluster-definition.json"), b, 0o600))
+			},
+			cleanup: func(t *testing.T) {
+				t.Helper()
+				err := os.RemoveAll(".charon")
+				require.NoError(t, err)
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if test.prepare != nil {
+				test.prepare(t)
+			}
+
+			cmd := newCreateCmd(newCreateDKGCmd(runCreateDKG))
+			if test.threshold != "" {
+				cmd.SetArgs([]string{"dkg", test.enr, test.feeRecipient, test.withdrawal, test.outputDir, test.threshold})
+			} else {
+				cmd.SetArgs([]string{"dkg", test.enr, test.feeRecipient, test.withdrawal, test.outputDir})
+			}
+
+			err := cmd.Execute()
+			if test.expectedErr != "" {
+				require.ErrorContains(t, err, test.expectedErr)
+			} else {
+				require.NoError(t, err)
+			}
+
+			if test.cleanup != nil {
+				test.cleanup(t)
+			}
+		})
+	}
+}

tbls/herumi.go

@@ -84,6 +84,10 @@
 	t.Helper()
 	var p bls.SecretKey
 
+	if threshold <= 1 {
+		return nil, errors.New("threshold has to be greater than 1")
+	}
+
 	if err := p.Deserialize(secret[:]); err != nil {
 		return nil, errors.Wrap(err, "cannot unmarshal bytes into Herumi secret key")
 	}
@@ -133,6 +137,10 @@
 func (Herumi) ThresholdSplit(secret PrivateKey, total uint, threshold uint) (map[int]PrivateKey, error) {
 	var p bls.SecretKey
 
+	if threshold <= 1 {
+		return nil, errors.New("threshold has to be greater than 1")
+	}
+
 	if err := p.Deserialize(secret[:]); err != nil {
 		return nil, errors.Wrap(err, "cannot unmarshal bytes into Herumi secret key")
 	}