Revise `secrets-dir` flag in the VC #5480

chong-he · 2024-03-25T12:47:31Z

Issue Addressed

Clean up VC secrets-dir flag and docs #5331

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)

dapplion

LGTM, definitely not the most UX friendly flag

michaelsproul · 2024-04-05T03:29:11Z

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

chong-he · 2024-04-16T13:00:50Z

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:

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.
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.
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

michaelsproul · 2024-04-18T05:09:14Z

book/src/validator-management.md

@@ -75,6 +77,8 @@ recap:

 ### 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. 


I think we should delete this line

book/src/validator-management.md

michaelsproul

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 · 2024-04-18T06:26:41Z

@Mergifyio queue

mergify · 2024-04-18T06:26:59Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 5c30afb

Redo this PR: - #5480 After a regression during the switch to `clap_derive`. - #6300 - Remove `conflicts_with` - Add test to prevent future regression

chong-he added 2 commits March 25, 2024 08:33

Update docs on secrets-dir

812e318

Hidden secrets-dir flag

cc614ca

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 approved these changes Mar 26, 2024

View reviewed changes

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 added 2 commits April 16, 2024 10:19

Remove conflicts_with

0735d87

Restore description

9013e6b

chong-he changed the title ~~Hidden secrets-dir flag in the VC~~ Revise secrets-dir flag in the VC Apr 16, 2024

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

make cli

e2af316

michaelsproul reviewed Apr 18, 2024

View reviewed changes

chong-he commented Apr 18, 2024

View reviewed changes

book/src/validator-management.md Outdated Show resolved Hide resolved

Update book/src/validator-management.md

784cf91

michaelsproul approved these changes Apr 18, 2024

View reviewed changes

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

mergify bot added a commit that referenced this pull request Apr 18, 2024

Merge of #5480

6d11662

This was referenced Apr 18, 2024

merge queue: embarking unstable (49617f3) and [#5588 + #5480] together #5599

Closed

Fix execution layer redundancy #5588

Merged

mergify bot merged commit 5c30afb into sigp:unstable Apr 18, 2024

This was referenced Apr 18, 2024

Clean up VC secrets-dir flag and docs #5331

Closed

secrets-dir flag does not work as intended #5600

Open

chong-he deleted the secret-dir branch June 16, 2024 13:54

michaelsproul mentioned this pull request Apr 6, 2025

Remove CLI conflict for secrets-dir and datadir #7265

Merged

mergify bot pushed a commit that referenced this pull request Apr 6, 2025

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Revise `secrets-dir` flag in the VC #5480

Revise `secrets-dir` flag in the VC #5480

Uh oh!

chong-he commented Mar 25, 2024 •

edited

Loading

Uh oh!

dapplion left a comment

Uh oh!

michaelsproul commented Apr 5, 2024

Uh oh!

chong-he commented Apr 16, 2024 •

edited

Loading

Uh oh!

michaelsproul Apr 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

michaelsproul left a comment

Uh oh!

michaelsproul commented Apr 18, 2024

Uh oh!

mergify bot commented Apr 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

		@@ -75,6 +77,8 @@ recap:

		### 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.

Revise secrets-dir flag in the VC #5480

Revise secrets-dir flag in the VC #5480

Uh oh!

Conversation

chong-he commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Apr 5, 2024

Uh oh!

chong-he commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelsproul Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Apr 18, 2024

Uh oh!

mergify bot commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Uh oh!

Revise `secrets-dir` flag in the VC #5480

Revise `secrets-dir` flag in the VC #5480

chong-he commented Mar 25, 2024 •

edited

Loading

chong-he commented Apr 16, 2024 •

edited

Loading

michaelsproul Apr 18, 2024 •

edited

Loading

mergify bot commented Apr 18, 2024 •

edited

Loading