Skip to content

Revise secrets-dir flag in the VC #5480

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.

Sign up for GitHub

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

Jump to bottom
Merged
mergify merged 6 commits into from
Apr 18, 2024
Merged

Revise secrets-dir flag in the VC #5480

mergify merged 6 commits into from
Apr 18, 2024

Conversation

@chong-he
Copy link
Member

@chong-he chong-he commented Mar 25, 2024 •
edited
Loading

Issue Addressed

Proposed Changes

The secrets-dir flag in the validator client is not meant to be used together when starting the VC, i.e., lighthouse vc --secrets-dir ./path will not direct the VC to read the keystore password from the ./path. Instead, the secrets-dir will be created automatically when using the lighthouse account validator create command (reference), or when using the --http-store-passwords-in-secrets-dir and import the keystore via HTTP API. Therefore, I think it is better to hide the flag, and I also change the description to avoid confusion, and to deprecate the flag in the future.

When the secrets-dir is created using the above methods, it will always follow the --datadir of the VC. For example if VC is started with lighthouse vc --datadir ./folder, then a folder named secrets will be created in the directory ./folder/secrets alongside with a separate folder for the validator client ./folder/validators. Hence, this line is remained:

lighthouse/validator_client/src/cli.rs

Line 76 in 5ce1619

.conflicts_with("datadir")

Users who do not wish to store the keystore password in the validator_definitions.yml file can use the field voting_keystore_password_path and point to a desired directory. An amendment was made in the Lighthouse book to highlight this.

Edit: See the comments here: #5480 (comment)

@chong-he chong-he added val-client Relates to the validator client binary ready-for-review The code is ready for review docs Documentation labels Mar 25, 2024
dapplion
dapplion approved these changes Mar 26, 2024
View reviewed changes
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM, definitely not the most UX friendly flag

@michaelsproul
Copy link
Member

michaelsproul commented Apr 5, 2024

Oh, this isn't what I intended when I opened #5331. I think we should document the behaviour of the secrets-dir flag more accurately, and remove the conflicts_with so it can be used more easily

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 5, 2024
@chong-he
Copy link
Member Author

chong-he commented Apr 16, 2024 •
edited
Loading

Oh, this isn't what I intended when I opened #5331. I think we should document the behaviour of the secrets-dir flag more accurately, and remove the conflicts_with so it can be used more easily

I have revised the PR by removing the .conflicts_with("datadir") on the lighthouse vc and lighthouse account validator create. I have tested and here are the applications of the flag --secrets-dir:

  1. With lighthouse vc --http --http-store-passwords-in-secrets-dir --datadir new --secrets-dir secrets
    We can have the datadir in one directory. During importing a new keystore via the HTTP API, the password file will be created automatically and stored under another directory set with --secrets-dir.

  2. With lighthouse vc –datadir new –secrets-dir secrets where the folder secrets contain validator passwords.
    During key discovery (i.e., empty validator_definitions.yml file), the VC is able to import validator keystores and loaded the validators, pointing the voting_keystore_password_path to --secrets-dir. This is provided that the keystores were created using lighthouse account validator create.

  3. With lighthouse account validator create :
    lighthouse --network mainnet account validator create --wallet-name wally --wallet-password wally.pass --count 6 --secrets-dir secrets --datadir new

    This however does not create the secrets folder under --secrets-dir. I have removed the .conflicts_with("datadir"), but the secrets folder is created in the same directory as datadir (i.e., in new/secrets). Although this is not a widely used function, but for transparency I will mention it here. I don’t think this is the expected outcome, as from the help text:

      --secrets-dir <SECRETS_DIR>
        The path where the validator keystore passwords will be stored. Defaults to ~/.lighthouse/{network}/secrets

    I will need some help here to fix this.

    It should be noted that if the validator_definitions.yml file already contains the keystore information, the --secrets-dir will not be effective. i.e., one cannot start the VC with lighthouse vc --secrets-dir and expects the VC to read the password from the --secrets-dir. This is another issue that will be opened afterwards. This usage is still desirable as one will not need to provide the voting_keystore_password_path in the validator_definitions.yml. This is helpful as entering voting_keystore_password_path could be manual, and inconvenient for a large number of validator keys. Having a flag --secrets-dir to direct the VC to read the password from the path solves this inconvenience. To achieve this, we would need the VC to allow not having both voting_keystore_password_pat and voting_keystore_password in the validator_definitions.yml, where currently either of these two fields is required: https://lighthouse-book.sigmaprime.io/validator-management.html#fields

@chong-he chong-he changed the title Hidden secrets-dir flag in the VC Revise secrets-dir flag in the VC Apr 16, 2024
@chong-he chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 16, 2024
michaelsproul
michaelsproul reviewed Apr 18, 2024
View reviewed changes
book/src/validator-management.md Outdated

### Automatic validator discovery

> Note: The description below only applies when the keystore files are created using the [`lighthouse account validator create`](./key-management.md) command, which has been deprecated.
Copy link
Member

@michaelsproul michaelsproul Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should delete this line

chong-he
chong-he commented Apr 18, 2024
View reviewed changes
michaelsproul
michaelsproul approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Thanks CK!

Lets open an issue for the lighthouse account create bug. I think we probably don't need to solve it if we deprecate the lighthouse account create command, but it's good to have a record of it until that deprecation is complete.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 18, 2024
@michaelsproul
Copy link
Member

michaelsproul commented Apr 18, 2024

@Mergifyio queue

@mergify
Copy link

mergify bot commented Apr 18, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 5c30afb

mergify bot added a commit that referenced this pull request Apr 18, 2024
@mergify
Merge of #5480
6d11662
This was referenced Apr 18, 2024
Closed
Merged
@mergify mergify bot merged commit 5c30afb into sigp:unstable Apr 18, 2024
This was referenced Apr 18, 2024
Closed
Open
@chong-he chong-he deleted the secret-dir branch June 16, 2024 13:54
@michaelsproul michaelsproul mentioned this pull request Apr 6, 2025
Merged
mergify bot pushed a commit that referenced this pull request Apr 6, 2025
@michaelsproul
Remove CLI conflict for secrets-dir and datadir (#7265)
e77fb01 
Redo this PR:

- #5480

After a regression during the switch to `clap_derive`.

- #6300


  - Remove `conflicts_with`
- Add test to prevent future regression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

@dapplion dapplion dapplion approved these changes

Assignees

No one assigned

Labels

docs Documentation ready-for-merge This PR is ready to merge. val-client Relates to the validator client binary

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chong-he @michaelsproul @dapplion