Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle duplicate keystores in import path without error #7842

Merged
merged 17 commits into from
Nov 17, 2020

Conversation

dv8silencer
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
Currently if you have two keystores for the same validator key in the import folder path, there will be failure during import. This is documented in #7828

In keymanager/imported/import.go:

	var privKeyBytes []byte
	var pubKeyBytes []byte
	for i := 0; i < len(keystores); i++ {
		privKeyBytes, pubKeyBytes, importsPassword, err = dr.attemptDecryptKeystore(decryptor, keystores[i], importsPassword)
		if err != nil {
			return err
		}
		privKeys[i] = privKeyBytes
		pubKeys[i] = pubKeyBytes
		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
	}

A map is used to detect duplicate keys and if so, stops progress and returns an error. One initial question that came to my mind was if this de-duplication is needed in this function and whether this is just for logging as there is de-duplication in a function called later. The duplicate-catching aspect of this function is needed as it ends up calling createAccountsKeystore() which will check to see if the importing keys are duplicates of the keys already in the keymanager but not necessarily if the keys being imported are matches of each other (eg. if wallet starts with 0 keys, and you import 10 identical keys, all 10 will make it into the wallet since they are not identical to keys already in the wallet if all you relied on was the downstream function createAccountsKeystore(). This function's de-duplication is needed).

This PR changes that piece of code so that a map is used to collect the keys [which inherently prevents duplicates] and then in the end, conversion is done to [][]byte so that it is in a form that is expected by createAccountsKeystore().

I think the reporting of duplicate keys being detected within the import folder is also good to keep so there is a Warn level message.

Which issues(s) does this PR fix?

Fixes #7828

Other notes for review

@rauljordan rauljordan merged commit 1a72733 into prysmaticlabs:master Nov 17, 2020
@dv8silencer dv8silencer deleted the dv8-i7828 branch November 17, 2020 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If the keystore folder has duplicate keys during accounts import, it'll exit with FATAL error
2 participants