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

charon combine should have --no-verify flag #2321

Closed
3 tasks
gsora opened this issue Jun 15, 2023 · 2 comments · Fixed by #2352
Closed
3 tasks

charon combine should have --no-verify flag #2321

gsora opened this issue Jun 15, 2023 · 2 comments · Fixed by #2352
Assignees
Labels
enhancement New feature or request protocol Protocol Team tickets

Comments

@gsora
Copy link
Collaborator

gsora commented Jun 15, 2023

🎯 Problem to be solved

In #2300 @sugh01 tried combining cluster keys whose lock file has been modified without re-generating the lock hash, hence invalid for charon run unless executed with --no-verify.

🛠️ Proposed solution

  • Core team consensus on the proposed solution

Implement the same --no-verify logic in charon combine as charon run, so that a cluster lock with wrong lock hash can still be combined.

At the moment combine checks that all the lock files yield the same SHA256 hash.

The logic must change: if --no-verify is specified, combine must check that all the validator public keys and its associated key shares are the same in all lock files involved, rather than checking the cluster lock hash.

🧪 Tests

✅ Acceptance criteria

Given a user with a non-verifiable cluster lock, resulted from out-of-band modifications after the DKG process
When the user tries to recombine the validator private keys with the combine command
And the user specifies --no-verify as combine parameter
And the validator public keys and shares are the same in all the specified cluster locks
Then combine combines the validator keys specified in the cluster lock successfully

@gsora gsora added the enhancement New feature or request label Jun 15, 2023
@gsora gsora self-assigned this Jun 15, 2023
@github-actions github-actions bot added the protocol Protocol Team tickets label Jun 15, 2023
@gsora gsora changed the title charon compose should have --no-verify flag charon combine should have --no-verify flag Jun 15, 2023
@xenowits
Copy link
Contributor

@gsora not sure if we should allow users to combine keys if their cluster locks are malformed?

@gsora
Copy link
Collaborator Author

gsora commented Jun 16, 2023

@xenowits #2300 is a great example: the cluster lock was modified because the withdrawal address was wrong, but since users needed the cluster to work it was executed with --no-verify.

Now that combine is available the users might want to reconstruct the validator key, create a new cluster with them but this time with the correct withdrawal data.

Without --no-verify on combine this operation is impossible: one would need the lock file state that yielded the original config hash, which might be hard to obtain.

Also the fact that config hash doesn't match doesn't imply a malformed cluster lock: in this case, while the config hash was different, the cluster lock was well-formed and worked fine with charon run --no-verify.

This is why combine must check that all the validator public keys and shares are the same in lock file set, regardless if the config hash matches: you can't recombine if your lock and my lock don't agree on the validator list, but you can if they're the same even if config hash doesn't match and you specified --no-verify on the CLI.

@gsora gsora linked a pull request Jun 23, 2023 that will close this issue
obol-bulldozer bot pushed a commit that referenced this issue Jun 23, 2023
This flag warns the user if any of the cluster manifest files fail hash verification.

It behaves like exactly like `charon run --no-verify`.

The code will not stop if a diverging set of validator keys is present in the cluster manifests, but will error if not all public keys yielded by the combined private keys are present in the validator keys set.

category: feature
ticket: #2321
xenowits pushed a commit that referenced this issue Jun 26, 2023
This flag warns the user if any of the cluster manifest files fail hash verification.

It behaves like exactly like `charon run --no-verify`.

The code will not stop if a diverging set of validator keys is present in the cluster manifests, but will error if not all public keys yielded by the combined private keys are present in the validator keys set.

category: feature
ticket: #2321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request protocol Protocol Team tickets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants