[Feature] enable/disable validator accounts #7746

fabdarice · 2020-11-08T03:43:18Z

What type of PR is this?
Feature

What does this PR do? Why is it needed?
This PR adds CLI commands to enable and disable accounts.
Disabled accounts won't be able to be used as validator credentials to attest to the network.
A user can re-enable previously disabled accounts making them valid again in the validator set.

This feature can facilitate moving validator keys from/to other clients or servers without having to use the current delete.

CLI example
validator accounts enable --enable-public-keys=0xdeafbeef,0xdeadbeef2
validator accounts disable --disable-public-keys=0xdeafbeef,0xdeadbeef2

Which issues(s) does this PR fix?

Fixes #7485

Other notes for review
Manual Tests:

… and disable feature

validator/accounts/accounts_activation.go

validator/keymanager/imported/keymanager.go

fabdarice · 2020-11-08T04:00:39Z

validator/accounts/accounts.go

+)
+
+// AccountConfig specifies parameters to run to delete, enable, disable accounts.
+type AccountConfig struct {


This might be overkill.

Happy to create an ActivationAccountConfig if we want to keep each config distinct.

This is OK, just concerned what the PublicKeys [][]byte are for, as it is not clear what to use them for. Could we specify DeletePublicKeys and EnablePublicKeys and DisablePublicKeys ? Better to repeat ourselves and be more explicit with these config structs

Also rename to AccountsConfig

Just to be clear @rauljordan, are you suggesting:

type AccountsConfig struct { Wallet *wallet.Wallet Keymanager keymanager.IKeymanager DeletePublicKeys [][]byte EnablePublicKeys [][]byte DisablePublicKeys [][]byte }

validator/accounts/accounts_activation.go

validator/keymanager/imported/keymanager.go

…ing format

validator/keymanager/imported/keymanager.go

…idator-keys

… a string)

validator/keymanager/imported/keymanager.go

rauljordan · 2020-11-09T17:20:20Z

validator/accounts/accounts_activation.go

@@ -0,0 +1,289 @@
+package accounts


Let's rename this file to accounts_enable_disable.go to align with the other files in the package

rauljordan · 2020-11-09T17:24:11Z

validator/accounts/accounts.go

+)
+
+// AccountConfig specifies parameters to run to delete, enable, disable accounts.
+type AccountConfig struct {


This is OK, just concerned what the PublicKeys [][]byte are for, as it is not clear what to use them for. Could we specify DeletePublicKeys and EnablePublicKeys and DisablePublicKeys ? Better to repeat ourselves and be more explicit with these config structs

validator/accounts/accounts_activation.go

rauljordan · 2020-11-09T17:25:20Z

validator/accounts/accounts_activation.go

+	"github.com/urfave/cli/v2"
+)
+
+func DisableAccountCli(cliCtx *cli.Context) error {


Suggested change

func DisableAccountCli(cliCtx *cli.Context) error {

func DisableAccountsCli(cliCtx *cli.Context) error {

rauljordan · 2020-11-09T17:30:22Z

validator/accounts/accounts.go

+)
+
+// AccountConfig specifies parameters to run to delete, enable, disable accounts.
+type AccountConfig struct {


Also rename to AccountsConfig

validator/keymanager/imported/keymanager.go

…lete distinct attributes

…counts list, backup and delete

…enable-disable-validator-keys

…bdarice/prysm into feature/enable-disable-validator-keys

…enable-disable-validator-keys

fabdarice · 2020-11-13T01:06:22Z

validator/keymanager/imported/keymanager.go

@@ -111,6 +113,7 @@ func NewKeymanager(ctx context.Context, cfg *SetupConfig) (*Keymanager, error) {
 func NewInteropKeymanager(_ context.Context, offset, numValidatorKeys uint64) (*Keymanager, error) {
 	k := &Keymanager{
 		accountsChangedFeed: new(event.Feed),
+		opts:                DefaultKeymanagerOpts(),


@rauljordan this was the fix for E2E

codecov · 2020-11-13T01:32:05Z

Codecov Report

Merging #7746 (1c2077d) into master (5fdb916) will decrease coverage by 0.14%.
The diff coverage is 45.81%.

@@            Coverage Diff             @@
##           master    #7746      +/-   ##
==========================================
- Coverage   62.09%   61.95%   -0.15%     
==========================================
  Files         431      432       +1     
  Lines       30451    30683     +232     
==========================================
+ Hits        18909    19010     +101     
- Misses       8601     8707     +106     
- Partials     2941     2966      +25

rauljordan · 2020-11-13T02:12:25Z

validator/accounts/accounts_enable_disable.go

+			formattedPubKeys[i] = fmt.Sprintf("%#x", bytesutil.Trunc(pubKeyBytes))
+		}
+		allAccountStr := strings.Join(formattedPubKeys, ", ")
+		if !cliCtx.IsSet(flags.DisablePublicKeysFlag.Name) {


Is this correct?

good catch.. strange, I did manually test with the flag too lol.

terencechain · 2020-11-13T02:24:17Z

validator/accounts/accounts_enable_disable.go

+	"github.com/urfave/cli/v2"
+)
+
+func DisableAccountsCli(cliCtx *cli.Context) error {


Is Godoc happy with these exported functions w/o functional comments?

Added docstring

…enable-disable-validator-keys

fabdarice added 6 commits November 6, 2020 13:44

add --enable --disable flags for validator accounts

754a223

refactor DeleteAccountConfig into AccountConfig to be used for enable…

d206458

… and disable feature

add disable flag for validator accounts

5692834

[wip] add method to disable account

4ad4d84

refactor account delete

c4f5262

add disable & enable with proper filters

4106f03

fabdarice commented Nov 8, 2020

View reviewed changes

validator/accounts/accounts_activation.go Outdated Show resolved Hide resolved

fabdarice commented Nov 8, 2020

View reviewed changes

validator/accounts/accounts_activation.go Outdated Show resolved Hide resolved

fabdarice commented Nov 8, 2020

View reviewed changes

validator/keymanager/imported/keymanager.go Show resolved Hide resolved

fabdarice commented Nov 8, 2020

View reviewed changes

validator/accounts/accounts_activation.go Outdated Show resolved Hide resolved

fabdarice commented Nov 8, 2020

View reviewed changes

validator/keymanager/imported/keymanager.go Show resolved Hide resolved

fabdarice added 3 commits November 8, 2020 13:33

fix keymanager unit tests

ead3b69

update DisabledPublicKeys to be a string instead of [][]byte

18fc81f

fix FetchValidatingPrivateKeys to only fetch active keys with new str…

2d25055

…ing format

fabdarice commented Nov 9, 2020

View reviewed changes

validator/keymanager/imported/keymanager.go Outdated Show resolved Hide resolved

fabdarice added 2 commits November 9, 2020 06:52

Merge remote-tracking branch 'origin' into feature/enable-disable-val…

1fe0ca9

…idator-keys

fix FetchValidationPrivateKeys with new DisabledPublicKeys format (as…

68fd90e

… a string)

rauljordan reviewed Nov 9, 2020

View reviewed changes

fabdarice added 10 commits November 10, 2020 07:50

Merge branch 'master' into feature/enable-disable-validator-keys

cfc28cb

rename file + update AccountsConfig to include Disable, Enable and De…

b8ae9f1

…lete distinct attributes

rename accounts_activation -> accounts_enable_disable

0641aca

revert changes from using string to [][]byte for DisabledPublicKeys

beb25d1

add FetchAllValidatingPublicKeys to preserve the functionality for ac…

ac3c8d0

…counts list, backup and delete

Merge branch 'master' into feature/enable-disable-validator-keys

2e11f9f

fix unit tests

c216a65

convert publickeys from [][]byte to str before passing it to pb message

fdd4c90

add unit tests for disable keys

34b7d8e

add unit tests for EnableAccounts

97af504

fabdarice marked this pull request as ready for review November 11, 2020 19:39

fabdarice mentioned this pull request Nov 12, 2020

Enable & Disable Account #7787

Closed

6 tasks

fabdarice added 9 commits November 11, 2020 16:41

Merge branch 'master' into feature/enable-disable-validator-keys

0ace8c6

Merge branch 'master' into feature/enable-disable-validator-keys

207824d

Merge branch 'master' of github.com:prysmaticlabs/prysm into feature/…

2dd09e6

…enable-disable-validator-keys

Merge branch 'feature/enable-disable-validator-keys' of github.com:fa…

e15155e

…bdarice/prysm into feature/enable-disable-validator-keys

add unit tests for FetchAllValidatingPublicKeys

f70c26a

Merge branch 'master' into feature/enable-disable-validator-keys

1328c69

fix keymanageropts for InteropKey

8ffd20d

Merge branch 'feature/enable-disable-validator-keys' of github.com:fa…

b1f0e0f

…bdarice/prysm into feature/enable-disable-validator-keys

Merge branch 'master' of github.com:prysmaticlabs/prysm into feature/…

1c2077d

…enable-disable-validator-keys

fabdarice commented Nov 13, 2020

View reviewed changes

fabdarice requested a review from rauljordan November 13, 2020 01:34

rauljordan reviewed Nov 13, 2020

View reviewed changes

Fix mistake for enable accounts

2e347d4

rauljordan previously approved these changes Nov 13, 2020

View reviewed changes

terencechain reviewed Nov 13, 2020

View reviewed changes

fabdarice added 2 commits November 13, 2020 03:33

Merge branch 'master' of github.com:prysmaticlabs/prysm into feature/…

188767f

…enable-disable-validator-keys

add docstring to DisableAccountsCli and EnableAccountsCli

e80f64a

fabdarice dismissed rauljordan’s stale review via e80f64a November 13, 2020 11:33

fabdarice requested review from rauljordan and terencechain November 13, 2020 11:34

fabdarice added 4 commits November 13, 2020 04:11

remove previous testnet and add toledo & pyrmont

d740459

Merge branch 'master' into feature/enable-disable-validator-keys

1619bc7

Merge branch 'master' into feature/enable-disable-validator-keys

f8c1577

Merge branch 'master' into feature/enable-disable-validator-keys

c613fd8

rauljordan approved these changes Nov 13, 2020

View reviewed changes

rauljordan merged commit 16bccf0 into prysmaticlabs:master Nov 13, 2020

fabdarice deleted the feature/enable-disable-validator-keys branch November 13, 2020 16:27

victorelec14 mentioned this pull request Mar 1, 2024

This key is already in use by validator on the Beacon chain! - could not make validator deposit: **** ALERT **** stader-labs/stader-node#192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] enable/disable validator accounts #7746

[Feature] enable/disable validator accounts #7746

fabdarice commented Nov 8, 2020 •

edited

fabdarice Nov 8, 2020

rauljordan Nov 9, 2020

rauljordan Nov 9, 2020

fabdarice Nov 9, 2020

rauljordan Nov 9, 2020

rauljordan Nov 9, 2020

rauljordan Nov 9, 2020

rauljordan Nov 9, 2020

rauljordan Nov 9, 2020

fabdarice Nov 13, 2020 •

edited

codecov bot commented Nov 13, 2020

rauljordan Nov 13, 2020

fabdarice Nov 13, 2020

terencechain Nov 13, 2020

fabdarice Nov 13, 2020

	func DisableAccountCli(cliCtx *cli.Context) error {
	func DisableAccountsCli(cliCtx *cli.Context) error {

[Feature] enable/disable validator accounts #7746

[Feature] enable/disable validator accounts #7746

Conversation

fabdarice commented Nov 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabdarice Nov 13, 2020 • edited

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2020

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabdarice commented Nov 8, 2020 •

edited

fabdarice Nov 13, 2020 •

edited