createdkg: validate inputs

cmd/createcluster.go

@@ -36,8 +36,9 @@ import (
 )
 
 const (
+	// zeroAddress is not owned by any user, is often associated with token burn & mint/genesis events and used as a generic null address.
+	// See https://etherscan.io/address/0x0000000000000000000000000000000000000000.
 	zeroAddress    = "0x0000000000000000000000000000000000000000"
-	deadAddress    = "0x000000000000000000000000000000000000dead"
 	defaultNetwork = "mainnet"
 	minNodes       = 4
 )
@@ -121,7 +122,6 @@ func runCreateCluster(ctx context.Context, w io.Writer, conf clusterConfig) erro
 	}
 
 	// Map prater to goerli to ensure backwards compatibility with older cluster definitions and cluster locks.
-	// TODO(xenowits): Remove the mapping later.
 	if conf.Network == eth2util.Prater {
 		conf.Network = eth2util.Goerli.Name
 	}
@@ -656,7 +656,7 @@ func validateDef(ctx context.Context, insecureKeys bool, keymanagerAddrs []strin
 		return err
 	}
 
-	if insecureKeys && isMainNetwork(network) {
+	if insecureKeys && isMainOrGnosis(network) {
 		return errors.New("insecure keys not supported on mainnet")
 	} else if insecureKeys {
 		log.Warn(ctx, "Insecure keystores configured. ONLY DO THIS DURING TESTING", nil)

cmd/createcluster_internal_test.go

@@ -113,8 +113,8 @@ func TestCreateCluster(t *testing.T) {
 			}
 
 			test.Config.InsecureKeys = true
-			test.Config.WithdrawalAddrs = []string{deadAddress}
-			test.Config.FeeRecipientAddrs = []string{deadAddress}
+			test.Config.WithdrawalAddrs = []string{zeroAddress}
+			test.Config.FeeRecipientAddrs = []string{zeroAddress}
 
 			if test.Config.Network == "" {
 				test.Config.Network = eth2util.Goerli.Name
@@ -317,8 +317,8 @@ func TestSplitKeys(t *testing.T) {
 				NumDVs:            1,
 				NumNodes:          minNodes,
 				Threshold:         3,
-				FeeRecipientAddrs: []string{deadAddress},
-				WithdrawalAddrs:   []string{deadAddress},
+				FeeRecipientAddrs: []string{zeroAddress},
+				WithdrawalAddrs:   []string{zeroAddress},
 				ClusterDir:        t.TempDir(),
 			},
 		},
@@ -454,8 +454,8 @@ func TestKeymanager(t *testing.T) {
 		KeymanagerAddrs:      addrs,
 		KeymanagerAuthTokens: authTokens,
 		Network:              eth2util.Goerli.Name,
-		WithdrawalAddrs:      []string{deadAddress},
-		FeeRecipientAddrs:    []string{deadAddress},
+		WithdrawalAddrs:      []string{zeroAddress},
+		FeeRecipientAddrs:    []string{zeroAddress},
 		Clean:                true,
 		InsecureKeys:         true,
 	}
@@ -528,8 +528,8 @@ func TestPublish(t *testing.T) {
 		NumNodes:          minNodes,
 		NumDVs:            1,
 		Network:           eth2util.Goerli.Name,
-		WithdrawalAddrs:   []string{deadAddress},
-		FeeRecipientAddrs: []string{deadAddress},
+		WithdrawalAddrs:   []string{zeroAddress},
+		FeeRecipientAddrs: []string{zeroAddress},
 		PublishAddr:       addr,
 		Publish:           true,
 		InsecureKeys:      true,

cmd/createdkg.go

@@ -79,22 +79,31 @@ func runCreateDKG(ctx context.Context, conf createDKGConfig) (err error) {
 		}
 	}()
 
+	// Map prater to goerli to ensure backwards compatibility with older cluster definitions.
+	// TODO(xenowits): Remove the mapping later.
+	if conf.Network == eth2util.Prater {
+		conf.Network = eth2util.Goerli.Name
+	}
+
+	if err = validateConfig(conf.Threshold, len(conf.OperatorENRs), conf.Network); err != nil {
+		return err
+	}
+
 	conf.FeeRecipientAddrs, conf.WithdrawalAddrs, err = validateAddresses(conf.NumValidators, conf.FeeRecipientAddrs, conf.WithdrawalAddrs)
 	if err != nil {
 		return err
 	}
 
+	if err = validateWithdrawalAddrs(conf.WithdrawalAddrs, conf.Network); err != nil {
+		return err
+	}
+
 	version.LogInfo(ctx, "Charon create DKG starting")
 
 	if _, err := os.Stat(path.Join(conf.OutputDir, "cluster-definition.json")); err == nil {
 		return errors.New("existing cluster-definition.json found. Try again after deleting it")
 	}
 
-	// Don't allow cluster size to be less than 4.
-	if len(conf.OperatorENRs) < minNodes {
-		return errors.New("insufficient operator ENRs (min = 4)")
-	}
-
 	var operators []cluster.Operator
 	for i, opENR := range conf.OperatorENRs {
 		_, err := enr.Parse(opENR)
@@ -113,20 +122,6 @@ func runCreateDKG(ctx context.Context, conf createDKGConfig) (err error) {
 		log.Warn(ctx, "Non standard `--threshold` flag provided, this will affect cluster safety", nil, z.Int("threshold", conf.Threshold), z.Int("safe_threshold", safeThreshold))
 	}
 
-	// Map prater to goerli to ensure backwards compatibility with older cluster definitions.
-	// TODO(xenowits): Remove the mapping later.
-	if conf.Network == eth2util.Prater {
-		conf.Network = eth2util.Goerli.Name
-	}
-
-	if !eth2util.ValidNetwork(conf.Network) {
-		return errors.New("unsupported network", z.Str("network", conf.Network))
-	}
-
-	if err := validateWithdrawalAddrs(conf.WithdrawalAddrs, conf.Network); err != nil {
-		return err
-	}
-
 	forkVersion, err := eth2util.NetworkToForkVersion(conf.Network)
 	if err != nil {
 		return err
@@ -166,22 +161,45 @@ func runCreateDKG(ctx context.Context, conf createDKGConfig) (err error) {
 	return nil
 }
 
+// validateWithdrawalAddrs returns an error if any of the provided withdrawal addresses is invalid.
 func validateWithdrawalAddrs(addrs []string, network string) error {
 	for _, addr := range addrs {
-		if _, err := eth2util.ChecksumAddress(addr); err != nil {
+		checksumAddr, err := eth2util.ChecksumAddress(addr)
+		if err != nil {
 			return errors.Wrap(err, "invalid withdrawal address", z.Str("addr", addr))
+		} else if checksumAddr != addr {
+			return errors.New("invalid checksummed address", z.Str("addr", addr))
 		}
 
 		// We cannot allow a zero withdrawal address on mainnet or gnosis.
-		if isMainNetwork(network) && addr == zeroAddress {
+		if isMainOrGnosis(network) && addr == zeroAddress {
 			return errors.New("zero address forbidden on this network", z.Str("network", network))
 		}
 	}
 
 	return nil
 }
 
-// isMainNetwork returns true if the network is either mainnet or gnosis.
-func isMainNetwork(network string) bool {
+// validateConfig returns an error if any of the provided config parameter is invalid.
+func validateConfig(threshold, numOperators int, network string) error {
+	if threshold > numOperators {
+		return errors.New("threshold cannot be greater than length of operators",
+			z.Int("threshold", threshold), z.Int("operators", numOperators))
+	}
+
+	// Don't allow cluster size to be less than 4.
+	if numOperators < minNodes {
+		return errors.New("insufficient operator ENRs (min = 4)")
+	}
+
+	if !eth2util.ValidNetwork(network) {
+		return errors.New("unsupported network", z.Str("network", network))
+	}
+
+	return nil
+}
+
+// isMainOrGnosis returns true if the network is either mainnet or gnosis.
+func isMainOrGnosis(network string) bool {
 	return network == eth2util.Mainnet.Name || network == eth2util.Gnosis.Name
 }

cmd/createdkg_internal_test.go

@@ -4,22 +4,28 @@ package cmd
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"path"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"github.com/obolnetwork/charon/eth2util"
 )
 
+const validEthAddr = "0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359" // Taken from https://eips.ethereum.org/EIPS/eip-55
+
 func TestCreateDkgValid(t *testing.T) {
 	temp := t.TempDir()
 
 	conf := createDKGConfig{
 		OutputDir:         temp,
 		NumValidators:     1,
 		Threshold:         3,
-		FeeRecipientAddrs: []string{deadAddress},
-		WithdrawalAddrs:   []string{deadAddress},
+		FeeRecipientAddrs: []string{validEthAddr},
+		WithdrawalAddrs:   []string{validEthAddr},
 		Network:           defaultNetwork,
 		DKGAlgo:           "default",
 		OperatorENRs: []string{
@@ -46,27 +52,39 @@ func TestCreateDkgInvalid(t *testing.T) {
 		errMsg string
 	}{
 		{
-			conf: createDKGConfig{OperatorENRs: append([]string{
-				"-JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
-			}, validENRs...)},
+			conf: createDKGConfig{
+				OperatorENRs: append([]string{
+					"-JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
+				}, validENRs...),
+				Network: defaultNetwork,
+			},
 			errMsg: "invalid ENR: missing 'enr:' prefix",
 		},
 		{
-			conf: createDKGConfig{OperatorENRs: append([]string{
-				"enr:JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
-			}, validENRs...)},
+			conf: createDKGConfig{
+				OperatorENRs: append([]string{
+					"enr:JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
+				}, validENRs...),
+				Network: defaultNetwork,
+			},
 			errMsg: "invalid ENR: invalid enr record, too few elements",
 		},
 		{
-			conf: createDKGConfig{OperatorENRs: append([]string{
-				"enrJG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
-			}, validENRs...)},
+			conf: createDKGConfig{
+				OperatorENRs: append([]string{
+					"enrJG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
+				}, validENRs...),
+				Network: defaultNetwork,
+			},
 			errMsg: "invalid ENR: missing 'enr:' prefix",
 		},
 		{
-			conf: createDKGConfig{OperatorENRs: append([]string{
-				"JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
-			}, validENRs...)},
+			conf: createDKGConfig{
+				OperatorENRs: append([]string{
+					"JG4QDKNYm_JK-w6NuRcUFKvJAlq2L4CwkECelzyCVrMWji4YnVRn8AqQEL5fTQotPL2MKxiKNmn2k6XEINtq-6O3Z2GAYGvzr_LgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKlO7fSaBa3h48CdM-qb_Xb2_hSrJOy6nNjR0mapAqMboN0Y3CCDhqDdWRwgg4u",
+				}, validENRs...),
+				Network: defaultNetwork,
+			},
 			errMsg: "invalid ENR: missing 'enr:' prefix",
 		},
 		{
@@ -123,7 +141,64 @@ func TestExistingClusterDefinition(t *testing.T) {
 		require.NoError(t, os.RemoveAll(outDir))
 	}()
 
+	var enrs []string
+	for i := 0; i < minNodes; i++ {
+		enrs = append(enrs, "enr:-JG4QG472ZVvl8ySSnUK9uNVDrP_hjkUrUqIxUC75aayzmDVQedXkjbqc7QKyOOS71VmlqnYzri_taV8ZesFYaoQSIOGAYHtv1WsgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKwwq_CAld6oVKOrixE-JzMtvvNgb9yyI-_rwq4NFtajIN0Y3CCDhqDdWRwgg4u")
+	}
+	enrArg := fmt.Sprintf("--operator-enrs=%s", strings.Join(enrs, ","))
+	feeRecipientArg := fmt.Sprintf("--fee-recipient-addresses=%s", validEthAddr)
+	withdrawalArg := fmt.Sprintf("--withdrawal-addresses=%s", validEthAddr)
+
 	cmd := newCreateCmd(newCreateDKGCmd(runCreateDKG))
-	cmd.SetArgs([]string{"dkg", "--operator-enrs=enr:-JG4QG472ZVvl8ySSnUK9uNVDrP_hjkUrUqIxUC75aayzmDVQedXkjbqc7QKyOOS71VmlqnYzri_taV8ZesFYaoQSIOGAYHtv1WsgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQKwwq_CAld6oVKOrixE-JzMtvvNgb9yyI-_rwq4NFtajIN0Y3CCDhqDdWRwgg4u", "--fee-recipient-addresses=0xa6430105220d0b29688b734b8ea0f3ca9936e846", "--withdrawal-addresses=0xa6430105220d0b29688b734b8ea0f3ca9936e846"})
+	cmd.SetArgs([]string{"dkg", enrArg, feeRecipientArg, withdrawalArg})
+
 	require.EqualError(t, cmd.Execute(), "existing cluster-definition.json found. Try again after deleting it")
 }
+
+func TestValidateWithdrawalAddr(t *testing.T) {
+	t.Run("ok", func(t *testing.T) {
+		addrs := []string{validEthAddr}
+		err := validateWithdrawalAddrs(addrs, eth2util.Goerli.Name)
+		require.NoError(t, err)
+	})
+
+	t.Run("invalid network", func(t *testing.T) {
+		err := validateWithdrawalAddrs([]string{zeroAddress}, eth2util.Mainnet.Name)
+		require.ErrorContains(t, err, "zero address forbidden on this network")
+	})
+
+	t.Run("invalid withdrawal address", func(t *testing.T) {
+		addrs := []string{"0xBAD000BAD000BAD"}
+		err := validateWithdrawalAddrs(addrs, eth2util.Gnosis.Name)
+		require.ErrorContains(t, err, "invalid withdrawal address")
+	})
+
+	t.Run("invalid checksum", func(t *testing.T) {
+		addrs := []string{"0x000BAD0000000BAD0000000BAD0000000BAD0000"}
+		err := validateWithdrawalAddrs(addrs, eth2util.Gnosis.Name)
+		require.ErrorContains(t, err, "invalid checksummed address")
+	})
+}
+
+func TestValidateConfig(t *testing.T) {
+	t.Run("invalid threshold", func(t *testing.T) {
+		threshold := 5
+		numOperators := 4
+		err := validateConfig(threshold, numOperators, "")
+		require.ErrorContains(t, err, "threshold cannot be greater than length of operators")
+	})
+
+	t.Run("insufficient ENRs", func(t *testing.T) {
+		threshold := 1
+		numOperators := 3
+		err := validateConfig(threshold, numOperators, "")
+		require.ErrorContains(t, err, "insufficient operator ENRs (min = 4)")
+	})
+
+	t.Run("invalid network", func(t *testing.T) {
+		threshold := 3
+		numOperators := 4
+		err := validateConfig(threshold, numOperators, "cosmos")
+		require.ErrorContains(t, err, "unsupported network")
+	})
+}

testutil/compose/define.go

@@ -23,8 +23,9 @@ import (
 	"github.com/obolnetwork/charon/eth2util/enr"
 )
 
-// zeroXDead is the 0x00..00dead Ethereum address.
-const zeroXDead = `"0x000000000000000000000000000000000000dead"`
+// zeroAddress is not owned by any user, is often associated with token burn & mint/genesis events and used as a generic null address.
+// See https://etherscan.io/address/0x0000000000000000000000000000000000000000.
+const zeroAddress = `"0x0000000000000000000000000000000000000000"`
 
 // Clean deletes all compose directory files and artifacts.
 func Clean(ctx context.Context, dir string) error {
@@ -129,8 +130,8 @@ func Define(ctx context.Context, dir string, conf Config) (TmplData, error) {
 			{"num_validators", fmt.Sprint(conf.NumValidators)},
 			{"operator_enrs", strings.Join(enrs, ",")},
 			{"threshold", fmt.Sprint(conf.Threshold)},
-			{"withdrawal_addresses", zeroXDead},
-			{"fee-recipient_addresses", zeroXDead},
+			{"withdrawal_addresses", zeroAddress},
+			{"fee-recipient_addresses", zeroAddress},
 			{"dkg_algorithm", "frost"},
 			{"output_dir", "/compose"},
 			{"network", eth2util.Goerli.Name},

testutil/compose/lock.go

@@ -41,8 +41,8 @@ func Lock(ctx context.Context, dir string, conf Config) (TmplData, error) {
 			{"split-keys-dir", splitKeysDir},
 			{"num-validators", fmt.Sprint(conf.NumValidators)},
 			{"insecure-keys", fmt.Sprintf(`"%v"`, conf.InsecureKeys)},
-			{"withdrawal-addresses", zeroXDead},
-			{"fee-recipient-addresses", zeroXDead},
+			{"withdrawal-addresses", zeroAddress},
+			{"fee-recipient-addresses", zeroAddress},
 			{"network", eth2util.Goerli.Name},
 		}}
 

testutil/compose/testdata/TestDockerCompose_define_dkg_template.golden

@@ -27,11 +27,11 @@
     },
     {
      "Key": "withdrawal_addresses",
-     "Value": "\"0x000000000000000000000000000000000000dead\""
+     "Value": "\"0x0000000000000000000000000000000000000000\""
     },
     {
      "Key": "fee-recipient_addresses",
-     "Value": "\"0x000000000000000000000000000000000000dead\""
+     "Value": "\"0x0000000000000000000000000000000000000000\""
     },
     {
      "Key": "dkg_algorithm",

testutil/compose/testdata/TestDockerCompose_define_dkg_yml.golden

@@ -16,8 +16,8 @@ services:
       CHARON_NUM_VALIDATORS: 1
       CHARON_OPERATOR_ENRS: enr:-HW4QEp-BLhP30tqTGFbR9n2PdUKWP9qc0zphIRmn8_jpm4BYkgekztXQaPA_znRW8RvNYHo0pUwyPEwUGGeZu26XlKAgmlkgnY0iXNlY3AyNTZrMaEDG4TFVnsSZECZXT7VqroFZdceGDRgSBn_nBf16dXdB48,enr:-HW4QEp-BLhP30tqTGFbR9n2PdUKWP9qc0zphIRmn8_jpm4BYkgekztXQaPA_znRW8RvNYHo0pUwyPEwUGGeZu26XlKAgmlkgnY0iXNlY3AyNTZrMaEDG4TFVnsSZECZXT7VqroFZdceGDRgSBn_nBf16dXdB48,enr:-HW4QEp-BLhP30tqTGFbR9n2PdUKWP9qc0zphIRmn8_jpm4BYkgekztXQaPA_znRW8RvNYHo0pUwyPEwUGGeZu26XlKAgmlkgnY0iXNlY3AyNTZrMaEDG4TFVnsSZECZXT7VqroFZdceGDRgSBn_nBf16dXdB48,enr:-HW4QEp-BLhP30tqTGFbR9n2PdUKWP9qc0zphIRmn8_jpm4BYkgekztXQaPA_znRW8RvNYHo0pUwyPEwUGGeZu26XlKAgmlkgnY0iXNlY3AyNTZrMaEDG4TFVnsSZECZXT7VqroFZdceGDRgSBn_nBf16dXdB48
       CHARON_THRESHOLD: 3
-      CHARON_WITHDRAWAL_ADDRESSES: "0x000000000000000000000000000000000000dead"
-      CHARON_FEE_RECIPIENT_ADDRESSES: "0x000000000000000000000000000000000000dead"
+      CHARON_WITHDRAWAL_ADDRESSES: "0x0000000000000000000000000000000000000000"
+      CHARON_FEE_RECIPIENT_ADDRESSES: "0x0000000000000000000000000000000000000000"
       CHARON_DKG_ALGORITHM: frost
       CHARON_OUTPUT_DIR: /compose
       CHARON_NETWORK: goerli