-
Notifications
You must be signed in to change notification settings - Fork 919
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
Add show-private-keys flag to accounts-v2 list #7487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this doesn't compile. Please take a look. Thanks!
func (dr *Keymanager) FetchValidatingPrivateKeys(ctx context.Context) ([][]byte, error) { | ||
dr.lock.RLock() | ||
privKeys := make([][]byte, len(dr.secretKeysCache)) | ||
pubKeys, err := dr.FetchValidatingPublicKeys(ctx) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "could not retrieve public keys") | ||
} | ||
for i, pk := range pubKeys { | ||
seckey, ok := dr.secretKeysCache[pk] | ||
if !ok { | ||
return nil, errors.New("Could not fetch private key") | ||
} | ||
privKeys[i] = seckey.Marshal() | ||
} | ||
dr.lock.RUnlock() | ||
return privKeys, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are codepaths here that could exit without unlocking this lock. Please defer the unlocking of this lock
func (dr *Keymanager) FetchValidatingPrivateKeys(ctx context.Context) ([][]byte, error) { | |
dr.lock.RLock() | |
privKeys := make([][]byte, len(dr.secretKeysCache)) | |
pubKeys, err := dr.FetchValidatingPublicKeys(ctx) | |
if err != nil { | |
return nil, errors.Wrap(err, "could not retrieve public keys") | |
} | |
for i, pk := range pubKeys { | |
seckey, ok := dr.secretKeysCache[pk] | |
if !ok { | |
return nil, errors.New("Could not fetch private key") | |
} | |
privKeys[i] = seckey.Marshal() | |
} | |
dr.lock.RUnlock() | |
return privKeys, nil | |
} | |
func (dr *Keymanager) FetchValidatingPrivateKeys(ctx context.Context) ([][]byte, error) { | |
dr.lock.RLock() | |
defer dr.lock.RUnlock() | |
privKeys := make([][]byte, len(dr.secretKeysCache)) | |
pubKeys, err := dr.FetchValidatingPublicKeys(ctx) | |
if err != nil { | |
return nil, errors.Wrap(err, "could not retrieve public keys") | |
} | |
for i, pk := range pubKeys { | |
seckey, ok := dr.secretKeysCache[pk] | |
if !ok { | |
return nil, errors.New("Could not fetch private key") | |
} | |
privKeys[i] = seckey.Marshal() | |
} | |
return privKeys, nil | |
} |
} | ||
publicKeys = append(publicKeys, bytesutil.ToBytes48(withdrawalKey.PublicKey().Marshal())) | ||
} | ||
return publicKeys, nil | ||
} | ||
|
||
// FetchWithdrawalPrivateKeys fetches the list of withdrawal private keys from the keymanager. | ||
func (dr *Keymanager) FetchWithdrawalPrivateKeys(ctx context.Context) ([][]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tests for this new method?
@@ -12,6 +12,25 @@ import ( | |||
keystorev4 "github.com/wealdtech/go-eth2-wallet-encryptor-keystorev4" | |||
) | |||
|
|||
// ExportPrivateKeys retrieves the secret keys for the specified public | |||
// keys in the function input. | |||
func (dr *Keymanager) ExportPrivateKeys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tests for this new method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this method used? Can we delete? Couldn't find any mentions of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was leftover from an old implementation cause you originally wanted to have a subcommand rather than a flag to accounts-v2 list. I forgot to remove it :(
@@ -349,6 +349,25 @@ func (dr *Keymanager) FetchValidatingPublicKeys(ctx context.Context) ([][48]byte | |||
return result, nil | |||
} | |||
|
|||
// FetchValidatingPrivateKeys fetches the list of private keys from the secret keys cache | |||
func (dr *Keymanager) FetchValidatingPrivateKeys(ctx context.Context) ([][]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tests for this new method?
} | ||
privKeys[i] = bytesutil.ToBytes32(seckey.Marshal()) | ||
} | ||
lock.RUnlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can exit and return before unlocking, to fix this, let's do this at the top of the function:
lock.RLock()
defer lock.RUnlock()
pubKeys := make([][]byte, len(publicKeys)) | ||
for i, pk := range publicKeys { | ||
pubKeyBytes := pk.Marshal() | ||
secretKey, ok := secretKeysCache[bytesutil.ToBytes48(pubKeyBytes)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not acquire a read lock, let's do
lock.RLock()
defer lock.RUnlock()
at the top
@@ -12,6 +12,25 @@ import ( | |||
keystorev4 "github.com/wealdtech/go-eth2-wallet-encryptor-keystorev4" | |||
) | |||
|
|||
// ExportPrivateKeys retrieves the secret keys for the specified public | |||
// keys in the function input. | |||
func (dr *Keymanager) ExportPrivateKeys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this method used? Can we delete? Couldn't find any mentions of it
Codecov Report
@@ Coverage Diff @@
## master #7487 +/- ##
==========================================
- Coverage 61.83% 61.71% -0.13%
==========================================
Files 422 421 -1
Lines 29527 29479 -48
==========================================
- Hits 18259 18193 -66
- Misses 8343 8371 +28
+ Partials 2925 2915 -10 |
This PR adds a --show-private-keys flag to
validator accounts-v2 list
so that if run on an HD wallet it shows the validating and withdrawal private keys, if run on a non-HD wallet it shows the validating private keys.It fixes #7387