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

derived removal pt 1 #3

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

derived removal pt 1 #3

wants to merge 1 commit into from

Conversation

michaelneuder
Copy link
Owner

WIP of removal of derived field in prysmaticlabs#9883. Builds off of prysmaticlabs#10155.

A few notes on what I am going for here:

  1. Delete the derived package from validator/keymanager/. I moved the only unique function from the derived keymanager RecoverAccountsFromMnemonic into the local keymanager package.
  2. Moved mnemonic.go, mnemonic_test.go, eip_test.go and tests from the derived keymanager type into keymanager_test.go. (I think there is some room to remove code duplication in the test by moving to a table-driven style--- e.g., TestLocalKeymanager_FetchValidatingPublicKeys and TestLocalKeymanager_FetchValidatingPublicKeys_FromMnemonic aren't that different, so there is a lot of duplicated code. This is out of scope for this PR, just a note for future).
  3. I added a boolean field to both the wallet and the local keymanager structs to indicate if it has the mnemonic feature enabled. The bool is accessed by EnableMnenomic(). I am still ironing out if this needs to be a field of both, or maybe just one of them.

This is where it gets a bit messier. The bulk of the derived specific logic lives in the validator/accounts/ dir. The general approach I am taking is to try to merge whatever derived specific logic there is into the local (imported) version of the code and guarding it behind a boolean check against EnableMnemonic().

For example in validator/accounts/accounts_list.go, we have the local (https://github.com/prysmaticlabs/prysm/blob/28e0dc5d09dc709730ae62922f7ee1a2a2c80cff/validator/accounts/accounts_list.go#L55) and derived (https://github.com/prysmaticlabs/prysm/blob/28e0dc5d09dc709730ae62922f7ee1a2a2c80cff/validator/accounts/accounts_list.go#L63) cases for printing the accounts. The goal is to merge these in a cohesive way so that we can remove the derived case all together. What I did was compare the two list functions (listLocalKeymanagerAccounts: https://github.com/prysmaticlabs/prysm/blob/28e0dc5d09dc709730ae62922f7ee1a2a2c80cff/validator/accounts/accounts_list.go#L94) and (listDerivedKeymanagerAccounts: https://github.com/prysmaticlabs/prysm/blob/28e0dc5d09dc709730ae62922f7ee1a2a2c80cff/validator/accounts/accounts_list.go#L152). These functions do mostly the same things, but there are a few specific derived aspects, which I added to the listLocalKeymanagerAccounts function behind a conditional:

if keymanager.EnableMnemonic() {
  validatingKeyPath := fmt.Sprintf(local.ValidatingKeyDerivationPathTemplate, i)
  fmt.Printf("%s %s\n", au.BrightCyan("[derivation path]").Bold(), validatingKeyPath)
}

I can go through the other files in validator/accounts/ and give them similar treatment, but wanted to sanity check this approach before overcommitting!

Copy link

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly I think it makes sense.
you are adding a new persistent flag in the wallet to see if mnemonic is enabled, merging derived into imported, and then using the flag to determine if it should receive other information. at a high level this makes sense to me , i do want to see the parsing aspects for web and peopel with existing wallets that have the derived keymanager type to not be affected.

@@ -157,7 +149,7 @@ func extractWalletCreationConfigFromCli(cliCtx *cli.Context, keymanagerKind keym
}
skipMnemonic25thWord := cliCtx.IsSet(flags.SkipMnemonic25thWordCheckFlag.Name)
has25thWordFile := cliCtx.IsSet(flags.Mnemonic25thWordFileFlag.Name)
if keymanagerKind == keymanager.Derived {
if keymanagerKind == keymanager.EnableMnemonic() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 this line didn't make sense to me isn't EnableMnemonic a boolean? why is it checking against keymanagerKind here

@@ -105,15 +104,6 @@ func BackupAccountsCli(cliCtx *cli.Context) error {
if err != nil {
return errors.Wrap(err, "could not backup accounts for local keymanager")
}
case keymanager.Derived:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing to consider is wallet migration, those who have the derived type should be parsed and swapped to local somehow with the flag if we want to go with it this way 🤔

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please look at validator/keymanager/types.go and how parsing works. one worry that I have is the possibility of having a direct/derived directory locally for an existing client. we need a way to safely merge or handle this for existing users. otherwise it will only use the direct file( representing imported) instead of realizing there is a derived filed at the wallet dir.

michaelneuder pushed a commit that referenced this pull request Apr 17, 2022
* Remove synced tips

use last valid hash in removing invalid nodes.

* add test

* Remove unused code

* More unused parameters

* Fix proposer boost

* terence's review #1

* Fix conflicts

* terence's review 2

* rename argument

* terence's review #3

* rename optimistic -> status

* Minor clean up

* revert loop variable change

* do not mark lvh as valid

Co-authored-by: terence tsao <terence@prysmaticlabs.com>
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.

2 participants