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.

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

cli/deposit: significantly simplify validator keystore path #1618

Merged
merged 7 commits into from
Apr 26, 2020
Merged

cli/deposit: significantly simplify validator keystore path #1618

merged 7 commits into from
Apr 26, 2020

Conversation

q9f
Copy link
Contributor

@q9f q9f commented Apr 19, 2020 •
edited
Loading

PR Description

currently the validator keys are stored in an unnecessary long path and file name, e.g.,

~/.opt/byz-f/teku q9-valid-path 7s
❯ ll ~/.local/share/teku/keystore/validator_0x8612758a1376713c90900c6681849f1e04b0bca243e00246657932822fd40589a8c55165fc3b480365bcce75fa854e5c/
total 16K
4.0K drwxr-xr-x  2 user users 4.0K Apr  8 21:25 ./
4.0K drwxr-xr-x 16 user users 4.0K Apr 19 14:58 ../
4.0K -rw-r--r--  1 user users  893 Apr  8 21:25 validator_0x8612758a1376713c90900c6681849f1e04b0bca243e00246657932822fd40589a8c55165fc3b480365bcce75fa854e5c.json
4.0K -rw-r--r--  1 user users  893 Apr  8 21:25 withdrawal_0x8dc343b8bfb07f5bc93d43299d65cf577dc76aa95473a5c2563a0149e15fb077eedd52a52f699943f60ebfa02d2f9745.json

this pull request proposes to simplify the path significantly for usability handling encrypted key files:

~/.opt/byz-f/teku q9-valid-path
❯ ll ~/.local/share/teku/keystore/validator_8612758/
total 16K
4.0K drwxr-xr-x  2 user users 4.0K Apr  8 21:25 ./
4.0K drwxr-xr-x 16 user users 4.0K Apr 19 14:58 ../
4.0K -rw-r--r--  1 user users  893 Apr  8 21:25 validator_8612758.json
4.0K -rw-r--r--  1 user users  893 Apr  8 21:25 withdrawal_8dc343b.json

Fixed Issue(s)

N/A

ajsutton
ajsutton reviewed Apr 20, 2020
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Thanks for this. It is definitely a neater layout - the primary aim in how this works is to avoid any chance of losing generated keys accidentally. @rain-on & @jakehaugen do you have any thoughts on this change? In particular, did we deliberately include the public key in the actual .json file names to so they didn't overlap if someone just collected them all up into a single directory?

@EdJoJob Would this change make things easier or more difficult with the automation you've been working on?

artemis/src/main/java/tech/pegasys/artemis/cli/deposit/EncryptedKeystoreWriter.java Outdated Show resolved Hide resolved
@usmansaleem
Copy link
Contributor

The primary reason to use public key suffix of bls keystore file name is to avoid overwriting of generated keystores; also if someone decided to move/copy them all in one folder.

Is the first 7 chars of pk meant to remain unique for all keys? With this approach, we should make sure to not overwrite if files already exist. Also, the pk(2, 9) should that be appended to validator/withdrawal.json to still keep them unique enough?

@EdJoJob
Copy link

EdJoJob commented Apr 20, 2020

At the moment this will have no impact on my existing automation, in that it is just about to be created 😁 Good timing as far as I am concerned

@q9f
Copy link
Contributor Author

q9f commented Apr 22, 2020 •
edited
Loading

Thanks, I decided to restore a trimmed version of the pubkey in the filename.

~/.opt/byz-f/teku q9-valid-path
❯ ll ~/.local/share/teku/keystore/validator_8612758/
total 16K
4.0K drwxr-xr-x  2 user users 4.0K Apr  8 21:25 ./
4.0K drwxr-xr-x 16 user users 4.0K Apr 19 14:58 ../
4.0K -rw-r--r--  1 user users  893 Apr  8 21:25 validator_8612758.json
4.0K -rw-r--r--  1 user users  893 Apr  8 21:25 withdrawal_8dc343b.json

@q9f
Copy link
Contributor Author

q9f commented Apr 26, 2020

Done :)

ajsutton
ajsutton approved these changes Apr 26, 2020
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this.

@ajsutton ajsutton merged commit 56a3b87 into Consensys:master Apr 26, 2020
@q9f q9f deleted the q9-valid-path branch April 27, 2020 07:37
NicolasMassart added a commit to NicolasMassart/teku that referenced this pull request Apr 27, 2020
@NicolasMassart
Squashed commit of the following:
f09e037 
commit 1cc3466
Author: Sally MacFarlane <sally.macfarlane@consensys.net>
Date:   Mon Apr 27 15:44:58 2020 +1000

    added subcommand in message (Consensys#1671)

    Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>

commit af1c82a
Author: bgravenorst <50852695+bgravenorst@users.noreply.github.com>
Date:   Mon Apr 27 11:38:15 2020 +1000

    Update /validator/block API description. (Consensys#1664)

    * Update API description.

    Signed-off-by: Byron Gravenorst <byron.gravenorst@consensys.net>

    * Address feedback.

    Signed-off-by: Byron Gravenorst <byron.gravenorst@consensys.net>

commit 6936481
Author: Paul Harris <paul.harris@consensys.net>
Date:   Mon Apr 27 10:35:52 2020 +1000

    network cli test cases (Consensys#1667)

    * added a test case to show network option taking a URL.

    * split out some other command line options tests into their own modules, and added config file parsing.

    Signed-off-by: Paul Harris <paul.harris@consensys.net>

commit f79f65f
Author: Adrian Sutton <adrian.sutton@consensys.net>
Date:   Mon Apr 27 07:15:18 2020 +1000

    Implement attestation gossip validation requirements (Consensys#1661)

commit 56a3b87
Author: Raw Pong Ghmoa <58883403+q9f@users.noreply.github.com>
Date:   Sun Apr 26 22:53:11 2020 +0200

    cli/deposit: significantly simplify validator keystore path (Consensys#1618)

commit 381b9b5
Author: Anton Nashatyrev <Nashatyrev@users.noreply.github.com>
Date:   Sat Apr 25 21:12:08 2020 +0300

    Optimize: batch BLS verification  (Consensys#1632)

    * Add batch BLS verification
    * Parallelize batch BLS

commit 340af2c
Author: Cem Ozer <cemozer2018@u.northwestern.edu>
Date:   Fri Apr 24 12:50:10 2020 -0400

    Remove headBockRoot from beaconBlocksByRange request (Consensys#1659)
NicolasMassart added a commit to NicolasMassart/teku that referenced this pull request Apr 27, 2020
@NicolasMassart
Squashed commit of the following:
51da1a2 
commit 1cc3466
Author: Sally MacFarlane <sally.macfarlane@consensys.net>
Date:   Mon Apr 27 15:44:58 2020 +1000

    added subcommand in message (Consensys#1671)

    Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>

commit af1c82a
Author: bgravenorst <50852695+bgravenorst@users.noreply.github.com>
Date:   Mon Apr 27 11:38:15 2020 +1000

    Update /validator/block API description. (Consensys#1664)

    * Update API description.

    Signed-off-by: Byron Gravenorst <byron.gravenorst@consensys.net>

    * Address feedback.

    Signed-off-by: Byron Gravenorst <byron.gravenorst@consensys.net>

commit 6936481
Author: Paul Harris <paul.harris@consensys.net>
Date:   Mon Apr 27 10:35:52 2020 +1000

    network cli test cases (Consensys#1667)

    * added a test case to show network option taking a URL.

    * split out some other command line options tests into their own modules, and added config file parsing.

    Signed-off-by: Paul Harris <paul.harris@consensys.net>

commit f79f65f
Author: Adrian Sutton <adrian.sutton@consensys.net>
Date:   Mon Apr 27 07:15:18 2020 +1000

    Implement attestation gossip validation requirements (Consensys#1661)

commit 56a3b87
Author: Raw Pong Ghmoa <58883403+q9f@users.noreply.github.com>
Date:   Sun Apr 26 22:53:11 2020 +0200

    cli/deposit: significantly simplify validator keystore path (Consensys#1618)

commit 381b9b5
Author: Anton Nashatyrev <Nashatyrev@users.noreply.github.com>
Date:   Sat Apr 25 21:12:08 2020 +0300

    Optimize: batch BLS verification  (Consensys#1632)

    * Add batch BLS verification
    * Parallelize batch BLS

commit 340af2c
Author: Cem Ozer <cemozer2018@u.northwestern.edu>
Date:   Fri Apr 24 12:50:10 2020 -0400

    Remove headBockRoot from beaconBlocksByRange request (Consensys#1659)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@usmansaleem usmansaleem usmansaleem left review comments

@ajsutton ajsutton ajsutton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@q9f @usmansaleem @EdJoJob @ajsutton