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 validator-manager #3502

Closed
wants to merge 151 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Aug 24, 2022

Issue Addressed

Addresses #2557

Proposed Changes

Adds the lighthouse validator-manager command, which provides:

  • lighthouse validator-manager create
  • lighthouse validator-manager import
    • Imports validators from a validators.json file to the VC via the HTTP API.
  • lighthouse validator-manager move
    • Moves validators from one VC to the other, utilizing only the VC API.

Additional Info

In 98bcb94 I've reduced some VC ERRO and CRIT warnings to WARN or DEBG for the case where a pubkey is missing from the validator store. These were being triggered when we removed a validator but still had it in caches. It seems to me that UnknownPubkey will only happen in the case where we've removed a validator, so downgrading the logs is prudent. All the logs are DEBG apart from attestations and blocks which are WARN. I thought having some logging about this condition might help us down the track.

In 856cd7e I've made the VC delete the corresponding password file when it's deleting a keystore. This seemed like nice hygiene. Notably, it'll only delete that password file after it scans the validator definitions and finds that no other validator is also using that password file.

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Aug 24, 2022
@paulhauner paulhauner changed the base branch from stable to unstable August 24, 2022 07:59
@paulhauner paulhauner force-pushed the validator-manager branch 3 times, most recently from 04c7e1a to 25b6313 Compare August 31, 2022 00:50
Co-authored-by: chonghe <44791194+chong-he@users.noreply.github.com>
@paulhauner
Copy link
Member Author

When using create with the flag --network goerli, the deposits.json file created will have the field:
Network_name: “prater”
I am not sure if we use this deposits.json to send the deposit, will it go through? I see that staking-deposit-cli no longer has “prater” as an option in the network selection.

I just checked this and the launchpad will accept "prater" for the Goerli network. I suspect it'll need to keep doing so to ensure it supports old versions of CLI tooling.

@jimmygchen
Copy link
Member

I just checked this and the launchpad will accept "prater" for the Goerli network. I suspect it'll need to keep doing so to ensure it supports old versions of CLI tooling.

Just dug up some old notes on this that may be relevant to the above, if it hasn't changed since I last read the launchpad code, I suspect it could even be any value 🙈 :

Launchpad doesn't actually use the fork_version, eth2_network_name, deposit_cli_version fields for computing signing root (for signature verification), these fields just need to be present in the file.

@paulhauner
Copy link
Member Author

I suspect it could even be any value

I did try (by accident) uploading a mainnet deposit JSON when it was in Goerli mode and I was rejected. So it's doing some verification at least!

@paulhauner
Copy link
Member Author

With this validator-manager feature, the feature account validator create and the info in: https://lighthouse-book.sigmaprime.io/key-management.html now appears to be obsolete. We have put a bold font on this page to encourage users to use staking-deposit-cli, which I think is mainly because the deposit file created using account validator create is not compatible with the staking launchpad.

Great suggestion @chong-he, thanks 🙏 I've added a deprecation notice in 5654966.

@paulhauner paulhauner added v4.4.1 ETA August 2023 ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 7, 2023
book/src/key-management.md Outdated Show resolved Hide resolved
Co-authored-by: chonghe <44791194+chong-he@users.noreply.github.com>
@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 7, 2023
@paulhauner
Copy link
Member Author

paulhauner commented Aug 7, 2023

This is ready to merge! I believe the test failures are unrelated and should be helped by #4574.

@paulhauner
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 7, 2023
## Issue Addressed

Addresses #2557

## Proposed Changes

Adds the `lighthouse validator-manager` command, which provides:

- `lighthouse validator-manager create`
    - Creates a `validators.json` file and a `deposits.json` (same format as https://github.com/ethereum/staking-deposit-cli)
- `lighthouse validator-manager import`
    - Imports validators from a `validators.json` file to the VC via the HTTP API.
- `lighthouse validator-manager move`
    - Moves validators from one VC to the other, utilizing only the VC API.

## Additional Info

In 98bcb94 I've reduced some VC `ERRO` and `CRIT` warnings to `WARN` or `DEBG` for the case where a pubkey is missing from the validator store. These were being triggered when we removed a validator but still had it in caches. It seems to me that `UnknownPubkey` will only happen in the case where we've removed a validator, so downgrading the logs is prudent. All the logs are `DEBG` apart from attestations and blocks which are `WARN`. I thought having *some* logging about this condition might help us down the track.

In 856cd7e I've made the VC delete the corresponding password file when it's deleting a keystore. This seemed like nice hygiene. Notably, it'll only delete that password file after it scans the validator definitions and finds that no other validator is also using that password file.
@bors
Copy link

bors bot commented Aug 7, 2023

Build failed:

@paulhauner
Copy link
Member Author

bors retry

bors bot pushed a commit that referenced this pull request Aug 7, 2023
## Issue Addressed

Addresses #2557

## Proposed Changes

Adds the `lighthouse validator-manager` command, which provides:

- `lighthouse validator-manager create`
    - Creates a `validators.json` file and a `deposits.json` (same format as https://github.com/ethereum/staking-deposit-cli)
- `lighthouse validator-manager import`
    - Imports validators from a `validators.json` file to the VC via the HTTP API.
- `lighthouse validator-manager move`
    - Moves validators from one VC to the other, utilizing only the VC API.

## Additional Info

In 98bcb94 I've reduced some VC `ERRO` and `CRIT` warnings to `WARN` or `DEBG` for the case where a pubkey is missing from the validator store. These were being triggered when we removed a validator but still had it in caches. It seems to me that `UnknownPubkey` will only happen in the case where we've removed a validator, so downgrading the logs is prudent. All the logs are `DEBG` apart from attestations and blocks which are `WARN`. I thought having *some* logging about this condition might help us down the track.

In 856cd7e I've made the VC delete the corresponding password file when it's deleting a keystore. This seemed like nice hygiene. Notably, it'll only delete that password file after it scans the validator definitions and finds that no other validator is also using that password file.
@bors
Copy link

bors bot commented Aug 8, 2023

Build failed:

@paulhauner
Copy link
Member Author

Seems like CI is having trouble making calls to HTTP servers 😕

bors retry

bors bot pushed a commit that referenced this pull request Aug 8, 2023
## Issue Addressed

Addresses #2557

## Proposed Changes

Adds the `lighthouse validator-manager` command, which provides:

- `lighthouse validator-manager create`
    - Creates a `validators.json` file and a `deposits.json` (same format as https://github.com/ethereum/staking-deposit-cli)
- `lighthouse validator-manager import`
    - Imports validators from a `validators.json` file to the VC via the HTTP API.
- `lighthouse validator-manager move`
    - Moves validators from one VC to the other, utilizing only the VC API.

## Additional Info

In 98bcb94 I've reduced some VC `ERRO` and `CRIT` warnings to `WARN` or `DEBG` for the case where a pubkey is missing from the validator store. These were being triggered when we removed a validator but still had it in caches. It seems to me that `UnknownPubkey` will only happen in the case where we've removed a validator, so downgrading the logs is prudent. All the logs are `DEBG` apart from attestations and blocks which are `WARN`. I thought having *some* logging about this condition might help us down the track.

In 856cd7e I've made the VC delete the corresponding password file when it's deleting a keystore. This seemed like nice hygiene. Notably, it'll only delete that password file after it scans the validator definitions and finds that no other validator is also using that password file.
@bors
Copy link

bors bot commented Aug 8, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Add validator-manager [Merged by Bors] - Add validator-manager Aug 8, 2023
@bors bors bot closed this Aug 8, 2023
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Addresses sigp#2557

Adds the `lighthouse validator-manager` command, which provides:

- `lighthouse validator-manager create`
    - Creates a `validators.json` file and a `deposits.json` (same format as https://github.com/ethereum/staking-deposit-cli)
- `lighthouse validator-manager import`
    - Imports validators from a `validators.json` file to the VC via the HTTP API.
- `lighthouse validator-manager move`
    - Moves validators from one VC to the other, utilizing only the VC API.

In 98bcb94 I've reduced some VC `ERRO` and `CRIT` warnings to `WARN` or `DEBG` for the case where a pubkey is missing from the validator store. These were being triggered when we removed a validator but still had it in caches. It seems to me that `UnknownPubkey` will only happen in the case where we've removed a validator, so downgrading the logs is prudent. All the logs are `DEBG` apart from attestations and blocks which are `WARN`. I thought having *some* logging about this condition might help us down the track.

In sigp@856cd7e I've made the VC delete the corresponding password file when it's deleting a keystore. This seemed like nice hygiene. Notably, it'll only delete that password file after it scans the validator definitions and finds that no other validator is also using that password file.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Addresses sigp#2557

Adds the `lighthouse validator-manager` command, which provides:

- `lighthouse validator-manager create`
    - Creates a `validators.json` file and a `deposits.json` (same format as https://github.com/ethereum/staking-deposit-cli)
- `lighthouse validator-manager import`
    - Imports validators from a `validators.json` file to the VC via the HTTP API.
- `lighthouse validator-manager move`
    - Moves validators from one VC to the other, utilizing only the VC API.

In 98bcb94 I've reduced some VC `ERRO` and `CRIT` warnings to `WARN` or `DEBG` for the case where a pubkey is missing from the validator store. These were being triggered when we removed a validator but still had it in caches. It seems to me that `UnknownPubkey` will only happen in the case where we've removed a validator, so downgrading the logs is prudent. All the logs are `DEBG` apart from attestations and blocks which are `WARN`. I thought having *some* logging about this condition might help us down the track.

In sigp@856cd7e I've made the VC delete the corresponding password file when it's deleting a keystore. This seemed like nice hygiene. Notably, it'll only delete that password file after it scans the validator definitions and finds that no other validator is also using that password file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v4.4.1 ETA August 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants