Repository navigation
feat(validator): voluntary exits, as an ethlambda voluntary-exit command and the keymanager /voluntary_exit route - #779
pablodeymo wants to merge 4 commits into
Conversation
…exit`
sub-command that signs one keystore's exit, asks for a confirmation phrase,
publishes it to a beacon node and waits for the chain to accept it, plus the
keymanager API's POST /eth/v1/validator/{pubkey}/voluntary_exit, which signs
without publishing. Exits sign under the capella fork version from deneb on
(EIP-7044), keyed on the fork at the exit's epoch as Lighthouse does, so a
pre-signed exit stays valid across later forks.
The beacon node API gains `validator` (one registry entry with its activation,
exit and withdrawable epochs) and `submit_voluntary_exit`, implemented for the
HTTP client, failover and the test mock. The keymanager context carries the
beacon node, signing context and clock the new route needs.
The client's three required flags become `Option`s clap still marks required,
because `subcommand_negates_reqs` only relaxes the parser: the derived
`from_arg_matches` refused `validator exit` on a plain `PathBuf` field.
🤖 Codex Code Review
Tests could not run because rustup attempted to write to a read-only directory. Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt |
🤖 Claude Code ReviewReview:
|
🤖 Gemini Code ReviewThis PR is well-structured and thoroughly tested, including EIP-7044 ( A few minor edge cases in
Automated review by Gemini · gemini-4-argon · custom prompt |
🤖 Grok Code ReviewThe voluntary-exit flow is careful in the right places (syncing refusal, pubkey/index binding, confirmation before publish, EIP-7044 called out, e2e coverage). A few consensus and CLI correctness gaps should be fixed before this is relied on for real exits.
The pubkey check in Automated review by Grok (xAI) · grok-4.7 · custom prompt |
🤖 DeepSeek Code ReviewOverall the PR is well structured and the EIP-7044 signing-domain logic looks correct. The CLI split, the response-pubkey check in
Automated review by DeepSeek · deepseek-v4-pro · custom prompt |
🤖 Kimi Code Review
The normal current/future-epoch path correctly implements EIP-7044. The confirmation flow, presign/no-publication separation, node-response pubkey check, and keymanager authentication coverage look good. Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt |
Conflicts were all two sides adding to the same place, and both are kept: - bin/ethlambda/src/main.rs: the `validator exit` dispatch beside the new `slashing-protection` command. - bin/ethlambda/src/validator.rs: `--init-slashing-protection` is read before `ValidatorOptions::required` consumes the options. - crates/validator: `KeymanagerContext` carries the exit route's `beacon_node`, `signing` and `clock` beside main's `definitions_unflushed` and `protection`, at its definition and at every site that builds one, the exit route's own test harness included.
…e rest of the review The signing domain was keyed on the fork at the exit's own epoch. A node verifies against the state that processes the exit: any deneb-or-later state uses the capella version whatever epoch the exit names (EIP-7044), so `--epoch 0`, or `?epoch=0` on the keymanager route, produced an exit every node on a chain past deneb rejects. The domain now follows the fork at max(current_epoch, exit.epoch), the earliest epoch a state can process the exit in, and before deneb mirrors `get_domain` (the processing state's previous version for an epoch before its fork). `sign_voluntary_exit` and `voluntary_exit_signing_root` take the current epoch for that. `ethlambda validator exit`: - The shard committee period is measured against max(current_epoch, exit.epoch), as the chain measures it, so an exit naming an old epoch is no longer refused for a validator that is eligible now. A pre-signed exit is still checked against the epoch it is signed for. - A future `--epoch` without `--presign` is refused before the prompt rather than by the node's 400 after it. - The wait for the chain is bounded at two epochs, ends on any status that carries an exit epoch (`active_slashed` and `withdrawal_*` included), and no longer turns a node error after the publish into a failed run: the exit went out, so the node is asked again next slot. - An aborted confirmation prompt, an empty stdin included, exits with a failing status. - The slots-to-seconds product no longer truncates through `as u32`, a JSON encoding failure is no longer reported as a malformed response, and the password is zeroized on drop like the secret it decrypts. The keymanager route's comment no longer claims the read lock is held across the beacon node round trip; a delete landing in between is the same 404. docs/cli.md follows all of the above and notes electra's pending-withdrawal refusal, which only the node can check. Tests: the domain for a pre-deneb epoch on a chain past deneb and for an epoch two forks back before it; the old-epoch, future-epoch and pre-signed eligibility cases; every terminal status of the wait, its bound, and an outage after the publish, with the mock answering as a chain that processed the exit instead of a racing flip; the keymanager route at `?epoch=0`; and an exit naming epoch 0 accepted by this node's Beacon API end to end.
|
/review |
MegaRedHand
left a comment
There was a problem hiding this comment.
No correctness issues found in the exit logic or the EIP-7044 domain handling. The tests against the STF's process_voluntary_exit domain and the end-to-end run against this node's Beacon API are good to have.
One change I want before this merges: the validator client's required flags must not become Options that are still marked required (see the comment on bin/ethlambda/src/validator.rs). Please keep them as plain required fields and give exit its own command instead.
The rest are nits, plus a heads-up on how this collides with #776.
| #[arg(long, value_delimiter = ',', required = true)] | ||
| pub(crate) beacon_nodes: Vec<String>, | ||
| pub(crate) beacon_nodes: Option<Vec<String>>, | ||
|
|
||
| /// Directory holding the EIP-2335 keystores and `validator_definitions.yml`. | ||
| /// Without that file, keystores in the Lighthouse layout | ||
| /// (`<0xpubkey>/voting-keystore.json`, password in | ||
| /// `--secrets-dir/<0xpubkey>`) are discovered and the file is written. | ||
| #[arg(long)] | ||
| pub(crate) validators_dir: PathBuf, | ||
| #[arg(long, required = true)] | ||
| pub(crate) validators_dir: Option<PathBuf>, | ||
|
|
||
| /// Directory holding one password file per keystore. | ||
| #[arg(long)] | ||
| pub(crate) secrets_dir: PathBuf, | ||
| #[arg(long, required = true)] | ||
| pub(crate) secrets_dir: Option<PathBuf>, |
There was a problem hiding this comment.
Please don't make these Options while keeping them required = true. I don't want flags that are optional in the type but required by clap.
Changing the type, then required() re-checking it at runtime, then subcommand_negates_reqs/args_conflicts_with_subcommands on the variant: all three are workarounds for fitting exit inside ValidatorOptions. Every future required client flag would have to repeat the pattern, and one that misses it fails with a runtime eyre error instead of a clap usage error.
Keep beacon_nodes, validators_dir and secrets_dir as plain Vec<String> / PathBuf, and make the exit its own top-level command, the way slashing-protection already is (e.g. ethlambda voluntary-exit ...). That keeps ethlambda validator --beacon-nodes ... exactly as it is today and removes all three workarounds.
There was a problem hiding this comment.
Done in 92a9d6b. The exit is its own top-level command, ethlambda voluntary-exit, wired like slashing-protection, with its options and entry point in bin/ethlambda/src/voluntary_exit.rs. beacon_nodes, validators_dir and secrets_dir are plain required fields again: bin/ethlambda/src/validator.rs is identical to main, so ethlambda validator --beacon-nodes ... is exactly as it was and a run without its flags is clap's usage error, not a runtime one.
| /// The flags clap requires of a client run, which only `validator exit` | ||
| /// may omit (see the struct's comment). A `None` here is a bug in the | ||
| /// command-line wiring, reported as such rather than unwrapped. | ||
| fn required(self) -> eyre::Result<(Vec<String>, PathBuf, PathBuf)> { |
There was a problem hiding this comment.
Goes away with the comment above: once the fields are plain required ones, clap guarantees them and there's nothing left to re-check at runtime.
There was a problem hiding this comment.
Gone in 92a9d6b, with the comment above: the file is back to main's.
| // `args_conflicts_with_subcommands`: and none of the client's flags may be | ||
| // given alongside it, since none applies. On the variant rather than on | ||
| // `ValidatorOptions`, where clap's derive does not pick them up. | ||
| #[command(subcommand_negates_reqs = true, args_conflicts_with_subcommands = true)] |
There was a problem hiding this comment.
Same here: with the exit as its own top-level command, neither setting is needed and Validator goes back to a plain variant.
There was a problem hiding this comment.
Gone in 92a9d6b: Validator is a plain variant again, and VoluntaryExit sits beside it. The tests that pinned the two settings are replaced by one that parses voluntary-exit as its own sub-command.
| /// | ||
| /// `current_epoch` is the chain's, by the caller's clock; see | ||
| /// [`Self::voluntary_exit_signing_root`] for why the domain needs it. | ||
| pub fn sign_voluntary_exit( |
There was a problem hiding this comment.
Heads-up on #776. This won't build against it, and the description's "the Web3Signer variant in #776 picks it up unchanged" doesn't hold:
- feat(validator): Web3Signer remote signing and the keymanager /eth/v1/remotekeys route #776 makes every
sign_*async over&RwLock<ValidatorStore>and replacessign_rootwithsign(store, pubkey, root, epoch, SigningBody). It also givesSigningContexta privateselection_proofsfield, so theSigningContext { .. }literals inexit.rs(lines 151 and 604),http_api/mod.rs:255andhttp_api/voluntary_exit.rs:178break too.git merge-treealso shows textual conflicts inbin/ethlambda/src/validator.rs,http_api/mod.rs,http_api/proposer.rs,lib.rs,signing.rsanddocs/cli.md. - feat(validator): Web3Signer remote signing and the keymanager /eth/v1/remotekeys route #776's
SigningBodyhas noVOLUNTARY_EXITvariant, so remote keys need one added.fork_info(epoch)would send the epoch's scheduled fork, not the EIP-7044 capella version this function signs under. If Web3Signer derives the root from thatfork_info, it signs a different root and feat(validator): Web3Signer remote signing and the keymanager /eth/v1/remotekeys route #776's post-signingverifiescheck rejects every remote exit. Worth confirming what Web3Signer does for exits after deneb when adding it. - Once rebased, the keymanager route should pass
&context.storeand letsigntake the guard, rather than holding a read guard across what becomes a network call.
There was a problem hiding this comment.
Agreed on all three; I checked them against the #776 branch. Nothing in this PR's code changes for it, but the description's claim was wrong and is corrected: it now says the two collide and what the second to land needs, which is an async sign_voluntary_exit over &RwLock<ValidatorStore>, SigningContext::new in place of the literals, the keymanager route passing &context.store so sign takes the guard, and a voluntary-exit SigningBody whose fork_info yields the EIP-7044 capella domain rather than the exit epoch's scheduled fork. Until that variant exists, exits are local keystores only. What Web3Signer does with an exit after deneb is still to be confirmed, and belongs with adding the variant.
| /// The password file is read the way `ValidatorStore::load` reads one: whole, | ||
| /// with a trailing newline trimmed, since an editor leaves one and a password | ||
| /// ending in a newline is not something anyone chose. | ||
| pub fn load_keystore( |
There was a problem hiding this comment.
Nit: this repeats the read → Keystore::from_json → read password → trim → decrypt → insert_secret steps from ValidatorStore::load. A shared helper that decrypts one keystore/password pair would keep the two from drifting (the trim and the reasoning behind it, or any later change to how passwords are read).
There was a problem hiding this comment.
Done in 92a9d6b: ValidatorStore::insert_keystore_file reads one keystore and password pair, and both ValidatorStore::load and the exit's load_keystore call it. The trim and its reasoning live there once, and the password is zeroized on drop on both paths now.
|
|
||
| let resolved = context | ||
| .beacon_node | ||
| .validator_indices(&[pubkey]) |
There was a problem hiding this comment.
Nit: this resolves the index through validator_indices, while the CLI path uses the new validator() (GET /eth/v1/beacon/states/head/validators/{pubkey}), so the two exit paths look the validator up through different endpoints. One of them can fail where the other succeeds. Using validator() here too would keep them on one path. Relatedly, HttpBeaconNode::validator hand-rolls its GET and error mapping; letting get_optional take the status that means "not found" would avoid a third copy.
There was a problem hiding this comment.
Done in 92a9d6b, both halves. The keymanager route resolves the validator through validator(), the lookup the command makes. get_optional takes the status that means "nothing" (a 204 for payload attestation data, a 404 here), so HttpBeaconNode::validator no longer hand-rolls its GET and error mapping.
…nd validator lookups Review follow-ups on the voluntary exit: - `ethlambda validator exit` becomes `ethlambda voluntary-exit`, a command of its own like `slashing-protection`. Living inside `validator` had turned the client's three required flags into `Option`s clap still marked required, re-checked at runtime by `ValidatorOptions::required`, with `subcommand_negates_reqs` and `args_conflicts_with_subcommands` on the variant to let the exit omit them. All three workarounds are gone: `bin/ethlambda/src/validator.rs` is as on main, and a client run without its flags is clap's usage error again rather than a runtime one. The command's options and entry point move to `voluntary_exit.rs`. - `ValidatorStore::insert_keystore_file` is the one place a keystore and its password file are read from disk, used by `ValidatorStore::load` and by the exit command, which had its own copy of the same steps. The password is zeroized on drop in both now. - The keymanager route resolves the validator through `validator()`, the lookup the command makes, instead of `validator_indices`, so the two ways to sign an exit cannot disagree about a validator's index. - `HttpBeaconNode::get_optional` takes the status that means "nothing", a 204 or a 404, so `validator()` no longer hand-rolls its GET and error mapping. docs/cli.md gives `voluntary-exit` its own section beside `slashing-protection`.
ethlambda validator exit sub-command and the keymanager /voluntary_exit routeethlambda voluntary-exit command and the keymanager /voluntary_exit route
🗒️ Description / Motivation
voluntary_exitroute, and this client had neither. The beacon node already accepts, validates, pools and gossips aSignedVoluntaryExit(POST /eth/v1/beacon/pool/voluntary_exits), but nothing on the validator side could produce one.ethlambda voluntary-exit, which signs one keystore's exit and hands it to a beacon node, andPOST /eth/v1/validator/{pubkey}/voluntary_exiton the keymanager API, which signs without publishing.What Changed
crates/validator/src/signing.rs:voluntary_exit_signing_rootandsign_voluntary_exit. The domain follows the fork of the state that processes the exit, the one atmax(current_epoch, exit.epoch), which both functions take the current epoch for. EIP-7044: from deneb on that isCAPELLA_FORK_VERSIONwhatever epoch the exit names, an epoch from before deneb included; before deneb it mirrorsget_domainon that state (its previous fork version for an epoch before its fork). Lighthouse keys on the exit's epoch alone, which signs--epoch 0under the genesis version on a chain past deneb, an exit no node accepts. Local keystores only: it goes through the privatesign_root, which feat(validator): Web3Signer remote signing and the keymanager /eth/v1/remotekeys route #776 replaces (see Related).crates/validator/src/beacon_node/: two newBeaconNodeApimethods.validator(pubkey)isGET /eth/v1/beacon/states/head/validators/{pubkey}with the activation, exit and withdrawable epochs (404 isNone; an answer naming another key isInconsistentResponse).submit_voluntary_exitisPOST /eth/v1/beacon/pool/voluntary_exits; a 400 surfaces asBeaconNodeStatuswith the node's reason. Implemented for the HTTP client,FallbackBeaconNodeand the test mock.crates/validator/src/exit.rs(new): the command. Refuses a syncing or optimistic node, then a future--epochwithout--presign, then a validator the chain would refuse (notactive_ongoing, or not yetSHARD_COMMITTEE_PERIODpast activation, naming the eligible epoch; the period is measured againstmax(current_epoch, exit.epoch), as the chain measures it), signs, and unless--no-confirmationasks for the phraseExit my validatoron stdin before publishing.--presignstops after signing and returns theSignedVoluntaryExit. Unless--no-wait, it then polls the validator once a slot, for up to two epochs, until the chain has given it an exit epoch:active_exiting(printing the current, exit and withdrawable epochs and the time to the exit epoch), any later status, oractive_slashed. A node error during the wait is retried at the next slot, and a wait that runs out returns successfully saying so, since the exit was published.run_withis the testable core over anyBeaconNodeApiand anyBufRead. The keystore and its password file are read byValidatorStore::insert_keystore_file, the one helperValidatorStore::loadnow uses too, so the command and the client cannot come to read a password differently.crates/validator/src/http_api/voluntary_exit.rs(new): the keymanager route.epochquery parameter (the spec'sUint64string) or the current epoch by the client's clock; 404 for a key not held and for a key the beacon node has no index for yet, through the samevalidator()lookup the command makes; 400 for a malformed key or epoch; the read lock is not held across the beacon node round trip, so a keystore delete landing in between is the same 404.KeymanagerContextgainsbeacon_node,signingandclock;parse_uint64inproposer.rsnow names the field it rejects.bin/ethlambda/src/voluntary_exit.rs(new),command.rs,main.rs:ethlambda voluntary-exit --keystore <path> --password-file <path> [--beacon-node URL] [--epoch N] [--presign] [--no-confirmation] [--no-wait], a top-level command of its own likeslashing-protection. Logs go to stderr so a pre-signed exit on stdout pipes cleanly.ethlambda validatorandbin/ethlambda/src/validator.rsare untouched.crates/validator/src/error.rs:ExitRefused, deterministic likeProposalRefused.slot_clock.rs:slot_duration().docs/cli.md: avoluntary-exitsection (flags, the phrase, what the wait prints, the EIP-7044 note) and the keymanager route.Correctness / Behavior Guarantees
voluntary_exit_signing_root, andone_object_root_signs_differently_under_each_domainstill covers the rest.process_voluntary_exitwould refuse, so an operator sees the reason rather than a node's 400; the node's gossip rules remain the authority and a refusal from them is reported verbatim.ethlambda validatoris exactly as onmain: its options, its required flags and its tests are not touched by this PR.--presignwith a future--epochproduces an exit a node would refuse today and accept once that epoch comes, which is the point of it. Without--presigna future epoch is refused before the prompt.Tests Added / Run
signing.rs: an exit verifies under its own root; at and after deneb it signs under the capella version and not the schedule's; an exit naming a pre-deneb epoch signs under capella on a chain past deneb; before deneb under what the processing state verifies, an epoch two forks back included.exit.rs: a syncing node, an unknown validator, an inactive one and one inside its shard committee period are refused with nothing published; an exit naming a past epoch publishes for an eligible validator; a future epoch without presign is refused; presign verifies and submits nothing; a wrong phrase submits nothing; the phrase publishes;--no-confirmationskips the prompt; the epoch defaults to the current one; the wait loop ends on every status that carries an exit epoch, gives up after its bound, and rides out a node outage after the publish (understart_paused).http_api/voluntary_exit.rs: the happy path returns a verifying signature with quoted integers;?epoch=is honoured;?epoch=0on a chain past deneb verifies under the capella domain; malformed epoch and pubkey are 400; a key not held and a key with no index are 404; no bearer token is 401.crates/net/rpc/src/beacon/validator_client_tests.rs: the whole command (minus the prompt) run against this node's Beacon API over a real socket: the lookup, the signature, the pool endpoint, the gossip publish, a 404 for an unknown key, a forged exit answered with the node's 400, and an exit naming epoch 0 accepted by the node.bin/ethlambda/src/command.rs:voluntary-exitparses as its own sub-command with its own flags, and is not given thenodedefault.cargo test -p ethlambda-validator --profile release-fast: 401 passed.cargo test -p ethlambda --profile release-fast: 266 passed.cargo test -p ethlambda-rpc --profile release-fast: 336 passed, the integration test among them.make lint: clean.cargo fmt --check: clean.Related Issues / PRs
sign_*async over&RwLock<ValidatorStore>, replacessign_rootwithsign(store, pubkey, root, epoch, SigningBody)and givesSigningContexta private field behindSigningContext::new.sign_voluntary_exit, theSigningContext { .. }literals here and the keymanager route's read guard all have to follow. Remote keys cannot sign exits untilSigningBodygains a voluntary-exit variant whosefork_infoyields the EIP-7044 capella domain rather than the exit epoch's scheduled fork; what Web3Signer does with it after deneb is still to be confirmed.--presignfor a validator that is not active yet (the keymanager route already signs for any held key with an index), a password prompt when--password-fileis absent (needs anrpassword-style dependency), exiting several validators in one call, and exiting through a running client's keymanager from the CLI (Lighthouse'svalidator-manager exit).process_voluntary_exitdomain.✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test— the validator crate, the binary and the rpc integration test were run atrelease-fast; CI runs the rest