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

cmd: add exit commands #2934

Merged
merged 16 commits into from
Apr 3, 2024
Merged

Conversation

gsora
Copy link
Collaborator

@gsora gsora commented Mar 3, 2024

This PR adds exit-related commands under the exit subcommand:

  • submit-partial-exit, which signs and submit to an instance of Obol API a partial exit for a given DV in a given cluster lock
  • broadcast, which downloads a full exit from an instance of Obol API for a given validator if available, and broadcasts it to the configured beacon node
  • active-validator-list, which returns the list of validators which are ACTIVE_ONGOING contained in the specified cluster lock (useful for scripting).

Moved obolapi mock implementation to testutil/obolapimock so other tests can use it.

Added a few utility functions in eth2util/keystore, taken from lido-dv-exit: since it depends on Charon, we can migrate them.

category: feature
ticket: #2848

Closes #2848.

This PR adds exit-related commands under the `exit` subcommand:

 - `submit-partial-exit`, which signs and submit to an instance of Obol API a partial exit for a given DV in a given cluster lock
 - `broadcast`, which downloads a full exit from an instance of Obol API for a given validator if available, and broadcasts it to the configured beacon node
 - `active-validator-list`, which returns the list of validators which are `ACTIVE_ONGOING` contained in the specified cluster lock (useful for scripting).

Moved `obolapi` mock implementation to `testutil/obolapimock` so other tests can use it.

Added a few utility functions in `eth2util/keystore`, taken from `lido-dv-exit`: since it depends on Charon, we can migrate them.
Copy link

codecov bot commented Mar 3, 2024

Codecov Report

Attention: Patch coverage is 44.21642% with 299 lines in your changes are missing coverage. Please review.

Project coverage is 53.96%. Comparing base (aa8a82e) to head (05d252d).
Report is 4 commits behind head on main.

❗ Current head 05d252d differs from pull request most recent head c101faa. Consider uploading reports for the commit c101faa to get more accurate results

Files Patch % Lines
cmd/exit.go 28.86% 64 Missing and 5 partials ⚠️
cmd/exit_broadcast.go 50.00% 49 Missing and 9 partials ⚠️
cmd/exit_fetch.go 32.50% 45 Missing and 9 partials ⚠️
cmd/exit_sign.go 52.21% 46 Missing and 8 partials ⚠️
cmd/exit_list.go 41.17% 36 Missing and 4 partials ⚠️
eth2util/keystore/keystore.go 67.85% 12 Missing and 6 partials ⚠️
cmd/cmd.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2934      +/-   ##
==========================================
- Coverage   54.16%   53.96%   -0.21%     
==========================================
  Files         195      200       +5     
  Lines       27564    28100     +536     
==========================================
+ Hits        14930    15164     +234     
- Misses      10876    11134     +258     
- Partials     1758     1802      +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gsora gsora requested a review from KaloyanTanev March 26, 2024 10:55
cmd/exit.go Show resolved Hide resolved
cmd/exit.go Show resolved Hide resolved
cmd/exit_list.go Show resolved Hide resolved
cmd/exit_fullexit_internal_test.go Outdated Show resolved Hide resolved
cmd/exit_list.go Outdated Show resolved Hide resolved
cmd/exit_submitpartial.go Outdated Show resolved Hide resolved
cmd/exit_submitpartial.go Outdated Show resolved Hide resolved
cmd/exit_submitpartial.go Outdated Show resolved Hide resolved
cmd/exit_fullexit.go Outdated Show resolved Hide resolved
cmd/exit_fullexit_internal_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@OisinKyne OisinKyne left a comment

Choose a reason for hiding this comment

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

Not all suggestions are musts. There might be some slight contradictions between them, use best judgement :)

cmd/exit_fetch.go Outdated Show resolved Hide resolved
cmd/exit_fetch.go Outdated Show resolved Hide resolved
"github.com/obolnetwork/charon/app/z"
)

func newListActiveValidatorsCmd(runFunc func(context.Context, exitConfig) error) *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider not adding this if we intend to remove (move) it under the get command?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather have it here for now, ops and support requested such function for a while now.

cmd/exit_sign.go Outdated Show resolved Hide resolved
cmd/exit.go Outdated Show resolved Hide resolved
cmd/exit.go Outdated Show resolved Hide resolved
cmd/exit_broadcast.go Outdated Show resolved Hide resolved
cmd/exit_sign.go Outdated Show resolved Hide resolved
@@ -225,3 +240,86 @@ func checkDir(dir string) error {

return nil
}

// KeysharesToValidatorPubkey maps each share in cl to the associated validator private key.
// It returns an error if a keyshare does not appear in cl, or if there's a validator public key associated to no
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// It returns an error if a keyshare does not appear in cl, or if there's a validator public key associated to no
// It returns an error if a keyshare does not appear in cl, or if there's a validator public key associated to no

Will we get pushback if charon needs all partials private keys returned to it, to broadcast the exit of a subset of them? This looks like it needs the full clusters worth to work? Whereas technically a user might bring back only the online keys to a charon command to sign exits for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm not really, this function maps a partial key share to its full validator pubkey by looking into the cluster lock, not by doing aggregation or any operation with the private key.

eth2util/keystore/keystore.go Outdated Show resolved Hide resolved
gsora and others added 8 commits April 3, 2024 10:57
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Co-authored-by: Oisín Kyne <[email protected]>
Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
21 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
24.7% Duplication on New Code

See analysis details on SonarCloud

@gsora
Copy link
Collaborator Author

gsora commented Apr 3, 2024

Seems to me we fixed all wording issues and the code looks okay.

Merging! 🎉

@gsora gsora added the merge when ready Indicates bulldozer bot may merge when all checks pass label Apr 3, 2024
@obol-bulldozer obol-bulldozer bot merged commit 2d662f0 into main Apr 3, 2024
11 checks passed
@obol-bulldozer obol-bulldozer bot deleted the gsora/exitcmd-submit-partial-exit branch April 3, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spike on generic exits
5 participants