Handle duplicate keystores in import path without error (#7842)

* bug fix

* Add regression test

* improve wording

* improve wording

* fix test

* comments, wording

* Comment

* import hex output

* fix test

* remove unnecessary sprintf

* fix test

Co-authored-by: dv8silencer <15720668+dv8silencer@users.noreply.github.com>

validator/accounts/accounts_import_test.go

@@ -80,6 +80,67 @@ func TestImport_Noninteractive(t *testing.T) {
 	assert.Equal(t, 2, len(keys))
 }
 
+// TestImport_DuplicateKeys is a regression test that ensures correction function if duplicate keys are being imported
+func TestImport_DuplicateKeys(t *testing.T) {
+	imported.ResetCaches()
+	walletDir, passwordsDir, passwordFilePath := setupWalletAndPasswordsDir(t)
+	keysDir := filepath.Join(t.TempDir(), "keysDir")
+	require.NoError(t, os.MkdirAll(keysDir, os.ModePerm))
+
+	cliCtx := setupWalletCtx(t, &testWalletConfig{
+		walletDir:           walletDir,
+		passwordsDir:        passwordsDir,
+		keysDir:             keysDir,
+		keymanagerKind:      keymanager.Imported,
+		walletPasswordFile:  passwordFilePath,
+		accountPasswordFile: passwordFilePath,
+	})
+	w, err := CreateWalletWithKeymanager(cliCtx.Context, &CreateWalletConfig{
+		WalletCfg: &wallet.Config{
+			WalletDir:      walletDir,
+			KeymanagerKind: keymanager.Imported,
+			WalletPassword: password,
+		},
+	})
+	require.NoError(t, err)
+	keymanager, err := imported.NewKeymanager(
+		cliCtx.Context,
+		&imported.SetupConfig{
+			Wallet: w,
+		},
+	)
+	require.NoError(t, err)
+
+	// Make sure there are no accounts at the start.
+	accounts, err := keymanager.ValidatingAccountNames()
+	require.NoError(t, err)
+	assert.Equal(t, len(accounts), 0)
+
+	// Create a key and then copy it to create a duplicate
+	_, keystorePath := createKeystore(t, keysDir)
+	time.Sleep(time.Second)
+	input, err := ioutil.ReadFile(keystorePath)
+	require.NoError(t, err)
+	keystorePath2 := filepath.Join(keysDir, "copyOfKeystore.json")
+	err = ioutil.WriteFile(keystorePath2, input, os.ModePerm)
+	require.NoError(t, err)
+
+	require.NoError(t, ImportAccountsCli(cliCtx))
+
+	w, err = wallet.OpenWallet(cliCtx.Context, &wallet.Config{
+		WalletDir:      walletDir,
+		WalletPassword: password,
+	})
+	require.NoError(t, err)
+	km, err := w.InitializeKeymanager(cliCtx.Context)
+	require.NoError(t, err)
+	keys, err := km.FetchValidatingPublicKeys(cliCtx.Context)
+	require.NoError(t, err)
+
+	// There should only be 1 account as the duplicate keystore was ignored
+	assert.Equal(t, 1, len(keys))
+}
+
 func TestImport_Noninteractive_RandomName(t *testing.T) {
 	imported.ResetCaches()
 	walletDir, passwordsDir, passwordFilePath := setupWalletAndPasswordsDir(t)

validator/keymanager/imported/import.go

@@ -23,31 +23,32 @@ func (dr *Keymanager) ImportKeystores(
 	importsPassword string,
 ) error {
 	decryptor := keystorev4.New()
-	privKeys := make([][]byte, len(keystores))
-	pubKeys := make([][]byte, len(keystores))
 	bar := initializeProgressBar(len(keystores), "Importing accounts...")
+	keys := map[string]string{}
 	var err error
-	var privKeyBytes []byte
-	var pubKeyBytes []byte
 	for i := 0; i < len(keystores); i++ {
+		var privKeyBytes []byte
+		var pubKeyBytes []byte
 		privKeyBytes, pubKeyBytes, importsPassword, err = dr.attemptDecryptKeystore(decryptor, keystores[i], importsPassword)
 		if err != nil {
 			return err
 		}
-		privKeys[i] = privKeyBytes
-		pubKeys[i] = pubKeyBytes
+		// if key exists prior to being added then output log that duplicate key was found
+		if _, ok := keys[string(pubKeyBytes)]; ok {
+			log.Warnf("Duplicate key in import folder will be ignored: %#x", pubKeyBytes)
+		}
+		keys[string(pubKeyBytes)] = string(privKeyBytes)
 		if err := bar.Add(1); err != nil {
 			return errors.Wrap(err, "could not add to progress bar")
 		}
 	}
-	foundKey := map[string]bool{}
-	for i := range pubKeys {
-		strKey := string(pubKeys[i])
-		if foundKey[strKey] {
-			return fmt.Errorf("duplicated key found: %#x", pubKeys[i])
-		}
-		foundKey[strKey] = true
+	privKeys := make([][]byte, 0)
+	pubKeys := make([][]byte, 0)
+	for pubKey, privKey := range keys {
+		pubKeys = append(pubKeys, []byte(pubKey))
+		privKeys = append(privKeys, []byte(privKey))
 	}
+
 	// Write the accounts to disk into a single keystore.
 	accountsKeystore, err := dr.createAccountsKeystore(ctx, privKeys, pubKeys)
 	if err != nil {

validator/keymanager/imported/import_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 	mock "github.com/prysmaticlabs/prysm/validator/accounts/testing"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
+	logTest "github.com/sirupsen/logrus/hooks/test"
 	keystorev4 "github.com/wealdtech/go-eth2-wallet-encryptor-keystorev4"
 )
 
@@ -103,20 +104,22 @@ func TestImportedKeymanager_ImportKeystores(t *testing.T) {
 		accountsStore: &accountStore{},
 	}
 
-	// Create a duplicate keystore and attempt to import it.
+	// Create a duplicate keystore and attempt to import it. This should complete correctly though log specific output.
 	numAccounts := 5
 	keystores := make([]*keymanager.Keystore, numAccounts+1)
 	for i := 1; i < numAccounts+1; i++ {
 		keystores[i] = createRandomKeystore(t, password)
 	}
 	keystores[0] = keystores[1]
 	ctx := context.Background()
-	require.ErrorContains(t, "duplicated key found:", dr.ImportKeystores(
+	hook := logTest.NewGlobal()
+	require.NoError(t, dr.ImportKeystores(
 		ctx,
 		keystores,
 		password,
 	))
-	// Import them correctly without the duplicate.
+	require.LogsContain(t, hook, "Duplicate key")
+	// Import them correctly even without the duplicate.
 	require.NoError(t, dr.ImportKeystores(
 		ctx,
 		keystores[1:],