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

Output all combined keystores to a single directory #1836

Closed
5 tasks
OisinKyne opened this issue Feb 27, 2023 · 5 comments Β· Fixed by #1876
Closed
5 tasks

Output all combined keystores to a single directory #1836

OisinKyne opened this issue Feb 27, 2023 · 5 comments Β· Fixed by #1876
Assignees
Labels
protocol Protocol Team tickets

Comments

@OisinKyne
Copy link
Contributor

OisinKyne commented Feb 27, 2023

🎯 Problem to be solved

We currently combine keys and write the keystores to subdirectories of the input folder given to charon combine. This structure is not friendly for importing into validator clients.

πŸ› οΈ Proposed solution

  • Approved design doc: link

  • Core team consensus on the proposed solution

  • Make an optional flag --output-dir (default: "validator_keys") that indicates the folder that keystores from charon combine are written to.

Here is the problem where this ticket arose from #1799 (comment)

πŸ§ͺ Tests

  • Tested by new automated unit/integration/smoke tests

πŸ‘ Additional acceptance criteria

  • Update the docs cli reference to match this new output structure.

❌ Out of Scope

If there is anything to highlight as out of scope for this issue, please outline it here.

@github-actions github-actions bot added the protocol Protocol Team tickets label Feb 27, 2023
@gsora
Copy link
Collaborator

gsora commented Mar 6, 2023

Right now the combine command outputs the following directories:

$ tree ./validators-to-be-combined

validators-to-be-combined/
β”œβ”€β”€ 0x822c5310674f4fc4ec595642d0eab73d01c62b588f467da6f98564f292a975a0ac4c3a10f1b3a00ccc166a28093c2dcd
β”‚   └── validator_keys
β”‚       β”œβ”€β”€ keystore-0.json
β”‚       └── keystore-0.txt
β”œβ”€β”€ 0x8929b4c8af2d2eb222d377cac2aa7be950e71d2b247507d19b5fdec838f0fb045ea8910075f191fd468da4be29690106
β”‚   └── validator_keys
β”‚       β”œβ”€β”€ keystore-0.json
β”‚       └── keystore-0.txt

that is, for each validator defined in the lock file one directory is created named after its public key and the private keys are placed in a subdirectory, called validator_keys.

This format is compatible with Charon and should be compatible with any other validator client that supports the Ethereum keystore format.

So @OisinKyne what you're suggesting is that instead of employing the directory structure I outlined earlier we do this:

$ tree ./validators-to-be-combined

validators-to-be-combined/
└── validator_keys
    β”œβ”€β”€ keystore-0.json
    β”œβ”€β”€ keystore-0.txt
    β”œβ”€β”€ keystore-1.json
    └── keystore-1.txt

Correct?

@OisinKyne
Copy link
Contributor Author

OisinKyne commented Mar 9, 2023

So @OisinKyne what you're suggesting is that instead of employing the directory structure I outlined earlier we do this:

$ tree ./validators-to-be-combined

validators-to-be-combined/
└── validator_keys
    β”œβ”€β”€ keystore-0.json
    β”œβ”€β”€ keystore-0.txt
    β”œβ”€β”€ keystore-1.json
    └── keystore-1.txt

Correct?

Yes but maybe we take an explicit (but with a default) output path, rather than placing the generated folder into the input folder, for fear of collisions or confusion or something.

Usage:
  charon combine [flags]

Flags:
      --cluster-dir string   Parent directory containing a number of .charon subdirectories from each node in the cluster. (default ".charon/cluster/")
      --force                Overwrites private keys with the same name if present.
  -h, --help                 Help for combine
       --output-dir string   Directory to output the combined private keys to. (default ./validator_keys/)

so if I made a folder ./distributed-validator-nodes-to-combine/, dumped all the .charon folders into it, and then from ./ I ran: charon combine --cluster-dir ./distributed-validator-nodes-to-combine/, it would spit out:

tree .
.
β”œβ”€β”€ distributed-validator-nodes-to-combine
β”‚   β”œβ”€β”€ node0
β”‚   β”‚   β”œβ”€β”€ charon-enr-private-key
β”‚   β”‚   β”œβ”€β”€ cluster-lock.json
β”‚   β”‚   β”œβ”€β”€ deposit-data.json
β”‚   β”‚   └── validator_keys
β”‚   β”‚       β”œβ”€β”€ keystore-0.json
β”‚   β”‚       └── keystore-0.txt
β”‚   β”œβ”€β”€ node1
β”‚   β”‚   β”œβ”€β”€ charon-enr-private-key
β”‚   β”‚   β”œβ”€β”€ cluster-lock.json
β”‚   β”‚   β”œβ”€β”€ deposit-data.json
β”‚   β”‚   └── validator_keys
β”‚   β”‚       β”œβ”€β”€ keystore-0.json
β”‚   β”‚       └── keystore-0.txt
β”‚   β”œβ”€β”€ node2
β”‚   β”‚   β”œβ”€β”€ charon-enr-private-key
β”‚   β”‚   β”œβ”€β”€ cluster-lock.json
β”‚   β”‚   β”œβ”€β”€ deposit-data.json
β”‚   β”‚   └── validator_keys
β”‚   β”‚       β”œβ”€β”€ keystore-0.json
β”‚   β”‚       └── keystore-0.txt
β”‚   └── node3
β”‚       β”œβ”€β”€ charon-enr-private-key
β”‚       β”œβ”€β”€ cluster-lock.json
β”‚       β”œβ”€β”€ deposit-data.json
β”‚       └── validator_keys
β”‚           β”œβ”€β”€ keystore-0.json
β”‚           └── keystore-0.txt
└── validator_keys
    β”œβ”€β”€ keystore-0.json
    └── keystore-0.txt

11 directories, 22 files

While you are doing this, can you make the default input directory .charon/cluster/, because that is the default output dir of charon create cluster for the time being. Thanks.

@gsora
Copy link
Collaborator

gsora commented Mar 13, 2023

Implemented, lmk if it works for you!

@OisinKyne
Copy link
Contributor Author

Suggested a variable change, once made all good to merge and close. (Though would be good to user test this ideally. Think eridian already made a guide).

@gsora
Copy link
Collaborator

gsora commented Mar 24, 2023

I'll go ahead, merge your suggestions and then merge in main.

Thanks!

obol-bulldozer bot pushed a commit that referenced this issue Mar 24, 2023
…mat (#1876)

This PR changes the `combine` command as follows:

- handle relative input paths properly
- make output path explicit with sane defaults
- combine keys in a single directory rather than one directory for each validator

category: refactor
ticket: #1836

Closes #1836.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol Protocol Team tickets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants