diff --git a/cmd/createcluster.go b/cmd/createcluster.go index fbe0e81cc..dd44993ff 100644 --- a/cmd/createcluster.go +++ b/cmd/createcluster.go @@ -144,7 +144,7 @@ func bindClusterFlags(flags *pflag.FlagSet, config *clusterConfig) { flags.StringSliceVar(&config.FeeRecipientAddrs, "fee-recipient-addresses", nil, "Comma separated list of Ethereum addresses of the fee recipient for each validator. Either provide a single fee recipient address or fee recipient addresses for each validator.") flags.StringSliceVar(&config.WithdrawalAddrs, "withdrawal-addresses", nil, "Comma separated list of Ethereum addresses to receive the returned stake and accrued rewards for each validator. Either provide a single withdrawal address or withdrawal addresses for each validator.") flags.IntVar(&config.NumDVs, "num-validators", 0, "The number of distributed validators needed in the cluster.") - flags.BoolVar(&config.SplitKeys, "split-existing-keys", false, "Split an existing validator's private key into a set of distributed validator private key shares. Does not re-create deposit data for this key.") + flags.BoolVar(&config.SplitKeys, "split-existing-keys", false, "Split an existing validator's private key into a set of distributed validator private key shares. Deposit data files are re-created using the provided withdrawal addresses; do not submit deposits for validators that are already active.") flags.StringVar(&config.SplitKeysDir, "split-keys-dir", "", "Directory containing keys to split. Expects keys in keystore-*.json and passwords in keystore-*.txt. Requires --split-existing-keys.") flags.StringVar(&config.PublishAddr, "publish-address", "https://api.obol.tech/v1", "The URL to publish the lock file to.") flags.BoolVar(&config.Publish, "publish", false, "Publish lock file to obol-api.") @@ -198,7 +198,14 @@ func runCreateCluster(ctx context.Context, w io.Writer, conf clusterConfig) erro // If SplitKeys wasn't set, we wouldn't have reached this part of code because validateCreateConfig() // would've already errored. if conf.SplitKeys { - useSequencedKeys := len(conf.WithdrawalAddrs) > 1 + // Load keys in strict file index order when per-validator addresses may differ, + // so that keystore-N.json is paired with the Nth withdrawal and fee recipient address. + useSequencedKeys := len(conf.WithdrawalAddrs) > 1 || len(conf.FeeRecipientAddrs) > 1 + if conf.DefFile != "" { + // A definition file replicates a single provided address for each validator, + // so only distinct addresses require strict key file ordering. + useSequencedKeys = hasDistinctAddrs(def.WithdrawalAddresses()) || hasDistinctAddrs(def.FeeRecipientAddresses()) + } secrets, err = getKeys(conf.SplitKeysDir, useSequencedKeys) if err != nil { @@ -421,10 +428,8 @@ func validateCreateConfig(ctx context.Context, conf clusterConfig) error { if conf.NumDVs != 0 { return errors.New("--num-validators not supported with --split-existing-keys, fix configuration flags") } - } else { - if conf.NumDVs == 0 && conf.DefFile == "" { // if there's a definition file, infer this value from it later - return errors.New("missing --num-validators flag") - } + } else if conf.NumDVs == 0 && conf.DefFile == "" { // if there's a definition file, infer this value from it later + return errors.New("missing --num-validators flag") } // Don't allow cluster size to be less than 3. @@ -615,7 +620,7 @@ func writeWarning(w io.Writer) { _, _ = sb.WriteString("\n") _, _ = sb.WriteString("***************** WARNING: Splitting keys **********************\n") _, _ = sb.WriteString(" Please make sure any existing validator has been shut down for\n") - _, _ = sb.WriteString(" at least 2 finalised epochs before starting the charon cluster,\n") + _, _ = sb.WriteString(" at least 2 finalized epochs before starting the charon cluster,\n") _, _ = sb.WriteString(" otherwise slashing could occur. \n") _, _ = sb.WriteString("****************************************************************\n") _, _ = sb.WriteString("\n") @@ -1172,6 +1177,17 @@ func writeLockToAPI(ctx context.Context, publishAddr string, lock cluster.Lock) return cl.LaunchpadURLForLock(lock), nil } +// hasDistinctAddrs returns true if addrs contains more than one distinct address (ignoring case). +func hasDistinctAddrs(addrs []string) bool { + for _, addr := range addrs { + if !strings.EqualFold(addr, addrs[0]) { + return true + } + } + + return false +} + // validateAddresses checks if we have sufficient addresses. It also fills addresses slices if only one is provided. // //nolint:revive // Not really confusing here as they are passed as arguments as well. diff --git a/cmd/createcluster_internal_test.go b/cmd/createcluster_internal_test.go index fa55b5192..b76f49b32 100644 --- a/cmd/createcluster_internal_test.go +++ b/cmd/createcluster_internal_test.go @@ -554,6 +554,7 @@ func TestSplitKeys(t *testing.T) { name string numSplitKeys int conf clusterConfig + expectKeyOrder bool expectedErrMsg string }{ { @@ -578,6 +579,19 @@ func TestSplitKeys(t *testing.T) { ClusterDir: t.TempDir(), }, }, + { + name: "split keys with distinct fee recipient addresses", + numSplitKeys: 3, + conf: clusterConfig{ + Name: "test split keys", + NumNodes: minNodes, + Threshold: 3, + FeeRecipientAddrs: []string{testutil.RandomETHAddress(), testutil.RandomETHAddress(), testutil.RandomETHAddress()}, + WithdrawalAddrs: []string{zeroAddress}, + ClusterDir: t.TempDir(), + }, + expectKeyOrder: true, + }, } for _, test := range tests { @@ -620,6 +634,16 @@ func TestSplitKeys(t *testing.T) { require.NoError(t, lock.VerifySignatures(nil)) require.Equal(t, test.numSplitKeys, lock.NumValidators) + + if test.expectKeyOrder { + // With per-validator addresses, keystore-N.json must map to the Nth validator + // so that fee recipient and withdrawal addresses are paired correctly. + for i, key := range keys { + pubkey, err := tbls.SecretToPublicKey(key) + require.NoError(t, err) + require.Equal(t, "0x"+hex.EncodeToString(pubkey[:]), lock.Validators[i].PublicKeyHex()) + } + } } }) } diff --git a/cmd/testdata/TestCreateCluster_splitkeys_output.golden b/cmd/testdata/TestCreateCluster_splitkeys_output.golden index ba3613c72..8e51f80a0 100644 --- a/cmd/testdata/TestCreateCluster_splitkeys_output.golden +++ b/cmd/testdata/TestCreateCluster_splitkeys_output.golden @@ -1,7 +1,7 @@ ***************** WARNING: Splitting keys ********************** Please make sure any existing validator has been shut down for - at least 2 finalised epochs before starting the charon cluster, + at least 2 finalized epochs before starting the charon cluster, otherwise slashing could occur. **************************************************************** diff --git a/cmd/testdata/TestCreateCluster_splitkeys_with_cluster_definition_output.golden b/cmd/testdata/TestCreateCluster_splitkeys_with_cluster_definition_output.golden index ba3613c72..8e51f80a0 100644 --- a/cmd/testdata/TestCreateCluster_splitkeys_with_cluster_definition_output.golden +++ b/cmd/testdata/TestCreateCluster_splitkeys_with_cluster_definition_output.golden @@ -1,7 +1,7 @@ ***************** WARNING: Splitting keys ********************** Please make sure any existing validator has been shut down for - at least 2 finalised epochs before starting the charon cluster, + at least 2 finalized epochs before starting the charon cluster, otherwise slashing could occur. ****************************************************************