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

[Merged by Bors] - Add new validator API for voluntary exit #4119

Closed
wants to merge 16 commits into from

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Mar 22, 2023

Issue Addressed

Addresses #4117

Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

TODO

  • Add submission to BN
  • Add flag to allow voluntary exit via the API
    • no longer needed now the VC doesn't submit exit directly
  • Additional verification / checks, e.g. if validator on same network as BN
    • to be done on client side
  • Potentially wait for the message to propagate and return some exit information in the response
    • not required
  • Update http tests
  • Update lighthouse book
    • not required if this endpoint makes it to the standard keymanager API

@jimmygchen jimmygchen changed the base branch from stable to unstable March 22, 2023 05:36
@jimmygchen jimmygchen requested a review from AgeManning March 22, 2023 06:25
@jimmygchen
Copy link
Member Author

jimmygchen commented Mar 22, 2023

Hi @AgeManning

I've done an basic implementation based on the existing voluntary exit logic from account manager. I have a few questions:

  • Is it necessary and possible here to check the validator is on the same network as the BN? I wonder if the BN already does some sort of checks. I think this may require passing the Eth2NetworkConfig to the http-api::Context, I'm checking if this is available in the VC.
    • UPDATE: Not required, this is already checked during VC startup
  • I think it may be useful to wait for the message to propagate (one slot?) and return additional info in the response (e.g. current_epoch, exit_epoch, withdrawal_epoch). Alternatively, we could get the UI to query the validator a slot later - this might be easier. What do you think?
    • UPDATE: best for the UI to query

@jimmygchen jimmygchen self-assigned this Mar 22, 2023
@jimmygchen
Copy link
Member Author

Looks like we may not need to submit the message to the BN with this API, we could return the signed voluntary exit object and leave it to the UI to publish to BN.

@jimmygchen jimmygchen removed the request for review from AgeManning March 22, 2023 10:30
@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress HTTP-API labels Mar 22, 2023
@AgeManning
Copy link
Member

Looking good!

@jimmygchen jimmygchen marked this pull request as ready for review March 23, 2023 06:13
@jimmygchen
Copy link
Member Author

The current implementation works, but I'd probably hold off further changes for now as we have an active discussion on this endpoint in the key manager API spec ethereum/keymanager-APIs#57

@jimmygchen
Copy link
Member Author

PR for keymanager-API spec: ethereum/keymanager-APIs#58

@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Mar 28, 2023
let validator_index = validator_store
.validator_index(&pubkey_bytes)
.ok_or_else(|| {
warp_utils::reject::custom_server_error(format!(
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be a 404 "not found" rather than a 500 "internal server error"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Updated, thanks!

@jimmygchen jimmygchen requested a review from paulhauner March 29, 2023 05:41
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Very nice, I used this a couple of days ago to produce a signed exit on Prater! It worked really well. I've just made a couple of very minor suggestions based on my experience.

Happy to merge this after they've been addressed!

.validator_index(&pubkey_bytes)
.ok_or_else(|| {
warp_utils::reject::custom_not_found(format!(
"Unable to find validator with public key: {}",
Copy link
Member

Choose a reason for hiding this comment

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

I was using this endpoint on Goerli on a VC with 5,000 validators on it (a pretty unreasonable amount of validators, I know). It turns out that it takes some time to obtain all the validator indices for all pubkeys when the VCs starts (on a fresh boot the VC only knows pubkeys but doesn't know validator indices). I was hitting this 404 and it took me a bit to figure out what was going on.

Here's a suggestion you're free to cherry-pick which would clarify things:

b1a1cbf

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @paulhauner, thanks for this, nice changes! 👍 I've cherry-picked your changes. I wasn't aware that the validator indices were fetched / available later - good to know!

@paulhauner paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Mar 31, 2023
@jimmygchen jimmygchen requested a review from paulhauner April 3, 2023 01:52
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 3, 2023
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Excellent, let's do it!

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 3, 2023
@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Apr 3, 2023
## Issue Addressed

Addresses #4117 

## Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

## TODO

- [x] ~~Add submission to BN~~ 
  - removed, see discussion in [keymanager API](ethereum/keymanager-APIs#58)
- [x] ~~Add flag to allow voluntary exit via the API~~ 
  - no longer needed now the VC doesn't submit exit directly
- [x] ~~Additional verification / checks, e.g. if validator on same network as BN~~ 
  - to be done on client side
- [x] ~~Potentially wait for the message to propagate and return some exit information in the response~~ 
  - not required
- [x] Update http tests
- [x] ~~Update lighthouse book~~ 
  - not required if this endpoint makes it to the standard keymanager API

Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
@bors bors bot changed the title Add new validator API for voluntary exit [Merged by Bors] - Add new validator API for voluntary exit Apr 3, 2023
@bors bors bot closed this Apr 3, 2023
@jimmygchen jimmygchen deleted the validator-exit-api branch April 4, 2023 06:29
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

Addresses sigp#4117 

## Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

## TODO

- [x] ~~Add submission to BN~~ 
  - removed, see discussion in [keymanager API](ethereum/keymanager-APIs#58)
- [x] ~~Add flag to allow voluntary exit via the API~~ 
  - no longer needed now the VC doesn't submit exit directly
- [x] ~~Additional verification / checks, e.g. if validator on same network as BN~~ 
  - to be done on client side
- [x] ~~Potentially wait for the message to propagate and return some exit information in the response~~ 
  - not required
- [x] Update http tests
- [x] ~~Update lighthouse book~~ 
  - not required if this endpoint makes it to the standard keymanager API

Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

Addresses sigp#4117 

## Proposed Changes

See ethereum/keymanager-APIs#58 for proposed API specification.

## TODO

- [x] ~~Add submission to BN~~ 
  - removed, see discussion in [keymanager API](ethereum/keymanager-APIs#58)
- [x] ~~Add flag to allow voluntary exit via the API~~ 
  - no longer needed now the VC doesn't submit exit directly
- [x] ~~Additional verification / checks, e.g. if validator on same network as BN~~ 
  - to be done on client side
- [x] ~~Potentially wait for the message to propagate and return some exit information in the response~~ 
  - not required
- [x] Update http tests
- [x] ~~Update lighthouse book~~ 
  - not required if this endpoint makes it to the standard keymanager API

Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP-API ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants