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

Prysm Validator Accounts Revamp Tracking #6220

Closed
16 tasks done
rauljordan opened this issue Jun 11, 2020 · 8 comments
Closed
16 tasks done

Prysm Validator Accounts Revamp Tracking #6220

rauljordan opened this issue Jun 11, 2020 · 8 comments
Labels
Priority: High High priority item Tracking Gotta Catch 'Em All

Comments

@rauljordan
Copy link
Contributor

rauljordan commented Jun 11, 2020

Background

Validator accounts management is the weakest part of our codebase as it stands today, providing very little in terms of UX, security, and documentation necessary for production. Among some of the questions we receive on a frequent basis are:

  • How can I move around my validator to a different computer? Is there something I need to do to move my withdrawal key to cold storage?
  • I had an account on a different computer I want to move to this computer, but the account had a different password
  • How can I list my public keys / private keys?
  • How can I regenerate my deposit data for my validator?
  • What is the keymanager? What are keymanageropts? What are all these flags? There is no documentation for any of this
  • Is there documentation for accounts management? How can I provide a remote signer?

TODOs

General

  • Split accounts v1/ and v2/ code into separate packages
  • Define and implement prysm validator accounts-v2 commands: new, list, import, export
  • Ensure all commands work in both interactive vs. non-interactive
  • Define and implement prysm validator wallet-v2 commands create, edit-config
  • Integrate into validator runtime
  • Add comprehensive accounts-v2 documentation to https://docs.prylabs.network
  • Support altona key creation with feature flags

Direct Keymanager (Non-HD)

  • Fully implement with proper tests and godocs
  • Print withdrawal keys to stdout instead of persisting them
  • Sanitize and enforce secure passwords
  • Allow for regeneration of deposit data
  • Allow adding validator keys while validator client is running Add validator keys while validator client is running #5202

Remote Keymanager

  • Implement a reference example using gRPC: see prysmaticlabs/remote-signer
  • Implement in Prysm, allowing for initialization of a new remote keymanager by passing in required options such as a grpc remote address and TLS cert paths
  • Add comprehensive tests and godocs

Derived Keymanager (HD hierarchical deterministic)

@rauljordan
Copy link
Contributor Author

One of our contributors, @rkapka, mentioned all of these useful points:

I have been thinking what other operations might be useful apart from merging ans splitting and here are my proposals:

  • Export - the user specifies a source directory, a target directory and a list of public keys k_1..k_n whose data should be exported from the source directory's database. A new database containing the exported data is created in the target directory.
  • Question: Should the exported data be removed from the source database? Maybe add a boolean flag to control this?
  • Import - the user specifies a source directory and a target directory. All data from the source database is copied to the target database.
  • Move - the user specifies a source directory, a target directory and a list of public keys k_1..k_n whose data should be moved from the source directory's database to the target directory's database.
  • Question: Should the moved data be removed from the source database? Maybe add a boolean flag to control this?
  • Split (k_1..k_n) - splitting currently creates one database for each source database's key. Add a flag that allows to specify keys whose data should be split from the rest. As a result, two new databases are created: one with data for keys k_1..k_n and one with data for the rest of the keys. The source database stays untouched (this behaviour will be consistent with the "regular" split).
  • Question: Splitting one database into N databases will be tedious in case N is not a very small number. The keys could be passed in a more complicated manner, e.g. [k_1..k_n],[k_x..k_y]. As an example, passing [k1,k2],[k3,k4,k5] would create three databases:
    data for k1 and k2
    data for k3, k4 and k5
    data for the rest of keys (if there are any left)
  • Export could be achieved using Move by creating a new database if the target directory does not contain one. Import could be achieved using Move by moving all keys over to the target database if no keys have been provided. I am wondering if it is better to have multiple operations, each doing their own thing, or a single powerful operation. Or maybe both?

@rauljordan
Copy link
Contributor Author

@mcdee has some excellent points in #5202 that suggest a keymanager should have the ability of frequently querying information of new keys being available. We can incorporate these ideas into allowing new keys at runtime.

@prestonvanloon
Copy link
Member

prestonvanloon commented Jun 19, 2020

@prestonvanloon
Copy link
Member

FYI: This issue includes EIP-2335 keystore format (see design doc above).

cc: @hwwhww

@Karmastic
Copy link
Contributor

Will this support removing validator keys in case we want to move the validator to another client?

@stefa2k
Copy link

stefa2k commented Jul 7, 2020

Will this support removing validator keys in case we want to move the validator to another client?

Moving to another ETH2 client implementation (Teku, Nethermind, ...) or to another Prysm client?
At runtime?

@prestonvanloon
Copy link
Member

@stefa2k @Karmastic The new accounts design will allow exporting to the common format for EIP-2335, which can be imported by any other client that supports that format.

@rauljordan
Copy link
Contributor Author

rauljordan commented Jul 22, 2020

Pending small items to wrap this up @0xKiwi

  • Rename accounts-v2 new to create for consistency
  • No mnemonics for direct, we should only show the hex private withdrawal key to stdout
  • Remove the CanUnlockAccounts property from a wallet. This was old before we had a nice way of differentiating the NewWallet for the different keymanager kinds IVAN
  • Implement list for remote keymanager (it should likely just fetch the validating public keys, assign unique petnames for them using shared/petnames
  • Abstract the generateDepositTransaction and logDepositTransaction functions in derived.go and direct.go into some common file, perhaps somewhere in shared/ IVAN
  • Reduce all the different ways of saving and reading to and from disk in wallet.go into something more general
  • Remove wallet.AccountNames() in wallet.go as it is only used by direct and should live in direct.go instead
  • Rename the list.go, new.go, import.go, export.go files to have the prefix accounts
  • Add nicer godoc to the accounts/v2/doc.go file
  • Comprehensively test every command to see if it works for the different keymanagers
  • Comprehensively go through the usage flags and try --help on every command to see if all the flags make sense (I think wallet create uses PasswordFileFlag for the wallet password but the usage says it the flag is for account passwords)
  • Ensure runtime works for direct keymanager (in progress, deposit sent)
  • Ensure runtime works for derived keymanager (in progress, deposit sent)
  • Ensure runtime works for remote keymanager (needs a remote signer setup, in progress, deposit sent)
  • Ensure we use filepath.Join and try everything on windows
  • Ensure end-user viewable errors don't display stack traces IVAN
  • Sanitize passwords from file inputs by trimming end newline chars IVAN
  • Ensure export/import only works for direct keymanager
  • Comprehensive tests for wallet create
  • BLS flags for altona

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item Tracking Gotta Catch 'Em All
Projects
None yet
Development

No branches or pull requests

9 participants