cmd: add combine command

[gsora]

This commit adds the combine command.

It accepts an input directory path containing all the private key shares of a DV, an output directory path, and a lockfile path.

While the output directory can be nonexistent (will be created automatically), input and lockfile paths must exist.

The result of this process is a keystore file containing the combined private key of the distributed validator.

category: feature
ticket: #1311

[codecov]

Codecov Report

Base: 54.99% // Head: 55.10% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (d21717a) compared to base (eeab76e).
Patch coverage: 50.30% of modified lines in pull request are covered.

❗ Current head d21717a differs from pull request most recent head 3107d1a. Consider uploading reports for the commit 3107d1a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1799      +/-   ##
==========================================
+ Coverage   54.99%   55.10%   +0.10%     
==========================================
  Files         169      170       +1     
  Lines       22137    22215      +78     
==========================================
+ Hits        12175    12242      +67     
- Misses       8396     8404       +8     
- Partials     1566     1569       +3     

Impacted Files	Coverage Δ
cmd/cmd.go	80.95% <0.00%> (-0.78%)	⬇️
cmd/combine.go	0.00% <0.00%> (ø)
combine/combine.go	60.29% <60.29%> (ø)
app/app.go	61.35% <0.00%> (+0.64%)	⬆️
core/tracker/tracker2/tracker.go	76.62% <0.00%> (+3.18%)	⬆️
app/vmock.go	79.48% <0.00%> (+6.66%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[corverroos]

suggest aligning with run flag: lock-file

[OisinKyne]

How does this work for e.g. a 10 validator, 4 node cluster? Do you put 40 files in a flat folder? or 4 subfolders? do we do any traversing/exploring subdirectories? Would be great if we could dump e.g. 4 .charon folders slightly renamed alongside one another, and our command is able by default to find the lockfile(s) and validator_keys folders.

[gsora]

I believe this command should be require some work from the user: to recombine all the keys, it's assumed some work is needed to arrange them in the correct order.

If we want to automate some part of this, we could (as you suggested) let the user provide the validator_keys directories renamed with the public key share, and do some directory traversing to recombine.

I still believe though that each validator recombination needs a separate invocation of the combine command.

[OisinKyne]

Mmmm similar to exits, I believe in practice we will see people mostly combine/exit all validators in a cluster or none. I think partial will be a rare edge case. With the scale of enterprise deployments, I don't think a separate invocation per key is scalable, everyone will DIY their own shell script for-loop.

I think @Battenfield should user test some options, but I think 'people will send their .charon folder to the combiner, the combiner will dump all n folders into one directory, having to rename each of them something other than .charon (awkward but feels unavoidable), and from there I think they should be able to run charon combine --data-dir ./this-folder-ive-prepared, from where we should be able to traverse it, assuming that each subdir is a .charon folder, and we can find ./this-folder-ive-prepared/*/cluster-lock.json in each of them, along with their enr so we know who is who, and even a ./this-folder-ive-prepared/*/validator_keys/* path that we can use to find hopefully all of the keystores we expect. Assuming we can find everything we need, get or create output-dir and spit out the combined keystores. This discovery should probably only need to find threshold number of operators rather than everyone in the lock file (e.g. 3 folders of stuff in a four node cluster, so that you can combine with the minimum number of participants).

[gsora]

everyone will DIY their own shell script for-loop

Correct, I implied users will do something similar... I don't see it as being completely unfeasible, and on the flipside depending on deployments needs it could actually be more comfortable.

I imagine a K8S deployment where validator keys are secrets, you have to script your way around the secrets API to mount directories in pods and have combine work with them anyway.

Regardless, I'm open to changes on this.

[OisinKyne]

Yeah, you make me also wonder should we add a keymanager-address flag like the DKG and create cluster command, that can push the combined keys into a web3 keymanager. (can make a separate ticket for this, too much scope creep)

I think humour me on this one and we go with this combine command attempting to combine every private key in the lockfile and warns/(errors and needs to be flag overridden) if it can't discover a threshold of the private keys and passwords it needs to complete.

[gsora]

Alright, let's go this way.

An issue that arises is usability.

Users don't know what's in their keystore (what share is represented by a given keystore file?), and even if this is an information that can be easily obtained by opening the JSON file, I'd say not everybody is willing to do so.

A thing we could do is the following:

• each operator creates a directory named after their ENR, and places all the JSON and txt keystore files in it
• one operator collects all the directories and places them in a parent directory, and then invokes the combine command pointing it to said directory, the lock file and an output directory
• the tool firstly makes sure that there are the same number of directories as the number of operators, and that those directories are named after the ENRs
• for each validator N, counting from 0 to NumValidators, combine reads keystore-N.{json, txt} and yields the public key, checking that it's contained in the N-th validator entry in the lock file
• assuming this is correct, it proceeds with the combination and saves the resulting private key in the destination directory, with the JSON and txt files named after the root public key
• runs this for each validator in the lock file

WDYT?

[gsora]

@corverroos @OisinKyne considering this process yields recombined private keys, I think it's better to write a file with the private key bytes to disk instead of our own keystore files: if a group of operator wants to recombine their private key(s), most probably they want to stop using charon, so writing keystore files seems a bit pointless.

Thoughts?

[OisinKyne]

@gsora the intention behind writing a keystore is to align with an ethereum standard here https://eips.ethereum.org/EIPS/eip-2335 This is a format supported by all clients. The idea is they would import these keystores into the clients as if they were exported from the eth2-staking-cli. Our .charon/validator_keys folder also matches that structure (albeit with key shares not 'normal' keys)

[gsora]

Ok, I’ll write keystore files then.

The rest of the UX is fine for you?

[gsora]

Removing the do-not-merge tag since the code is ready for prime time.

@OisinKyne I added some docs on what the combine command expects from the user in order to work on the function doc itself, I'll replicate them in the obol-docs as soon as I have an ACK on the UX.

[gsora]

@OisinKyne instead of creating a single validator_keys output directory, I create one for each validator named after its public key.

I implemented what you suggested wrt the input directory tree (aka it's compatible with .charon directory scheme).

Is that fine?

[OisinKyne]

@OisinKyne instead of creating a single validator_keys output directory, I create one for each validator named after its public key.

I implemented what you suggested wrt the input directory tree (aka it's compatible with .charon directory scheme).

Is that fine?

Have you tried importing private keys in that format into validator clients? I don't believe that structure would be recognised. See:
https://lighthouse-book.sigmaprime.io/validator-import-launchpad.html#instructions
https://chainsafe.github.io/lodestar/usage/validator-management/#option-1-import-keys-to-lodestars-keystores-folder
https://docs.teku.consensys.net/Reference/CLI/CLI-Syntax#validator-keys
https://docs.prylabs.network/docs/wallet/nondeterministic#import-validator-accounts

I think only one validator_keys folder, files starting with keystore- and .json/.txt for file and password are all important conventions.

Second thing I want to check is that the format charon create cluster outputs can immediately work with charon combine ... will do now

[OisinKyne]

Okay this is way old, was under the impression this was changed, will make tickets for this

[gsora]

I think only one validator_keys folder, files starting with keystore- and .json/.txt for file and password are all important conventions.

Oh I see!

Ok, I'll move the json and txt file down under a validator_keys directory, then I'd say we're good to go with the merge.

[OisinKyne]

Suggested change

	flags.StringVar(keystoresDir, "keystores-dir", "./", `Directory containing all the keystore files organized by ENR, and the lock file.`)
	flags.StringVar(keystoresDir, "cluster-dir", ".charon/cluster/", `Parent directory containing a number of .charon subdirectories from each node in the cluster.`)

--cluster-dir aligns with charon create cluster, see no keystores-dir in our code yet.

Not sure about the default folder, maybe it could be ./obol/, will post something in #dev or see if chris has a strong product opinion.

[gsora]

I would push for ./.charon to keep it aligned with all the other cli commands.

[OisinKyne]

Haven't looked at the encompassing error handling around this function, but can a partial success happen where we don't manage to combine all keys successfully but do output some? We probably don't want to allow that, and should fail entirely if that's not already the case. (assume we freak out if err != nil

[gsora]

Hmm, the function will error out if one of the validator keys can't be combined but the artifacts of the other keys will remain.

[gsora]

I'm merging this PR, let's address this behavior in a separate one if needs a change.

[OisinKyne]

Should we allow --no-verify flags on this command @corverroos ? Imagined use case would be trying to recombine a really old cluster in a modern (>v1.0) charon version, and not being able to get it to verifySignatures correctly? (Or we can just pinky promise to always support old signatures/verification for anything that could possibly be in prod?)

Happy to skip this if not a concern

[gsora]

I'm merging this PR, let's address this behavior in a separate one if needs a change.

[OisinKyne]

Just want a slight change to the output format, a couple of tweaks to the explainer text.

For now, I've suggested a different default folder path, I don't like what I've suggested as .charon/cluster, but that is where the default create goes, so I think we should put the default combine there too, and fix both shortly together.