[pablodeymo]

🗒️ Description / Motivation

• The keymanager API's feerecipient, graffiti and gas_limit routes overrode one validator's settings in memory only: a restart put every validator back on the command line's defaults, fee recipient included, and nothing told the operator. Lighthouse persists the three on each entry of validator_definitions.yml; Teku, Prysm and Lodestar read a proposer-config file. This was the last high-severity item in the client-gap list.
• This persists them in validator_definitions.yml, in Lighthouse's exact layout, so a file written by either client reads in the other, and the keymanager, the file and startup agree on one record.

What Changed

• crates/validator/src/keys/definitions.rs: ValidatorDefinition gains suggested_fee_recipient (0x hex), graffiti (text) and gas_limit (integer), each #[serde(default, skip_serializing_if = "Option::is_none")], so an existing file parses unchanged and an entry without overrides is written exactly as before. overrides() parses the three (a malformed value is a Keystore error naming the entry and the field), apply(SettingChange) writes one change in the file's form, with_overrides_of carries them across a rebuilt entry, and ValidatorDefinitions::seed_overrides installs every enabled entry's into a ProposerSettings.
• crates/validator/src/proposer_settings.rs: Overrides is public, with SettingChange (one change to one of the three, None meaning back to the default), ProposerSettings::apply, seed and overrides; the six set_*/clear_* are now wrappers. fee_recipient_from_text/to_text moved here from the route (the zero-address refusal with them) so the file and the API parse an address one way. The module doc no longer says "in memory only": the file is the record, this is the cache.
• crates/validator/src/http_api/proposer.rs: every POST/DELETE goes through one persist_and_apply: definitions_lock first (the order the keystore routes take), store read lock for the held check, open the file, apply the change to the key's entry, save, then apply in memory. A failed read or write is a 500 and memory is unchanged, so what the operator sees is what the next restart loads. A held key with no entry in the file (the file was edited under a running client) is a 500 naming the file rather than a memory-only change. GETs are unchanged.
• crates/validator/src/http_api/keystores.rs: a re-imported key (password rotation, retry) rebuilt its entry from the new keystore and would have dropped the overrides; they are carried over. A fresh import starts with none.
• crates/validator/src/lib.rs: after the keys load, the definitions are opened once more and seeded into the settings; a malformed override refuses startup, as a bad keystore does.
• docs/cli.md: the keymanager section's "held in memory only" paragraph replaced with how they persist, that the fields may be written by hand, and that a Lighthouse file reads as is.

Correctness / Behavior Guarantees

• A definitions file written before this change parses unchanged, and one this client writes for keys with no overrides is byte-for-byte what it wrote before, so a reader predating the fields still reads it.
• Persist-before-apply: no state is ever in memory that is not in the file, except the pre-existing in-memory state of a client whose file was edited underneath it, which the routes now refuse to extend.
• Lock order is the keystore routes' (definitions_lock, then store), so the settings routes cannot deadlock against an import or delete and their open-mutate-save cycles never interleave.
• A disabled entry keeps its fields in the file and is not seeded, matching ValidatorStore::load skipping its key.
• The zero address is still refused on POST feerecipient (400), now from the shared parser, and is refused in the file too.
• Lighthouse's other fields (description, builder_*, type) are read and ignored, as every unknown key already was.

Tests Added / Run

• definitions.rs: an entry without overrides is written without the fields; overrides round-trip through the file; a Lighthouse-written entry (with its extra fields) parses with its three overrides; a malformed address and a 33-byte graffiti are errors naming the field; clearing removes the field; seeding installs enabled entries' overrides only.
• proposer_settings.rs: seeding replaces a key's overrides wholesale; the address parser round-trips and refuses the zero address and malformed input.
• http_api/proposer.rs: every POST lands in the file and every DELETE removes the field; persisted overrides survive a restart (a fresh ProposerSettings seeded from the file answers them); a key missing from the file is a 500 with memory unchanged; an unwritable directory is a 500 with memory and file unchanged (unix, skipped as root).
• http_api/keystores.rs: a re-imported key keeps its persisted graffiti and the file holds one entry.
• cargo test -p ethlambda-validator --profile release-fast: 339 passed. cargo test -p ethlambda --profile release-fast: 264 passed. make lint: clean. cargo fmt --check: clean.

Related Issues / PRs

• feat(validator): Web3Signer remote signing and the keymanager /eth/v1/remotekeys route #776 (Web3Signer) rewrites ValidatorDefinition's path fields into a flattened signing enum and reworks keystores.rs; the three fields here are additive and self-contained, so whichever lands second has a line-adjacent conflict in the struct and the import's Undo::Replaced arm, nothing structural.
• Out of scope, as follow-ups: a Teku/Prysm/Lodestar-style --proposer-settings-file, Lighthouse's builder_* fields, and a --graffiti-file.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test — the validator crate and the binary were run at release-fast (603 tests); CI runs the rest

[github-actions]

🤖 Grok Code Review

I'll inspect the surrounding keymanager and definitions code so the review is grounded in how locks, saves, and startup seeding actually work.

---

Automated review by Grok (xAI) · grok-4.7 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: persisting per-validator proposer overrides in validator_definitions.yml

The design is sound. Writing the file before changing memory under definitions_lock, and failing startup on a malformed override, is the right model. Test coverage is good. I found a few correctness gaps and some smaller items.

Correctness

1. A re-imported disabled entry leaves the file and memory out of sync (keystores.rs, Some(index) arm, ~L347)

with_overrides_of carries the old entry's overrides into the file. ProposerSettings is only updated by startup seed_overrides, and that skips disabled entries.

The failing case: the key was a disabled entry with, say, suggested_fee_recipient set by hand, and the operator imports it via the API. Then:

• The file says the override applies.
• Memory runs on the default until the next restart.

That breaks the module's own invariant: "what the operator sees is what the next restart loads." Fix by calling context.settings.seed(&pubkey, definition.overrides()?) after the save succeeds. Parse first, so a malformed carried-over field fails the import rather than the next startup.

2. persist_and_apply can write to the wrong entry (proposer.rs, the .find(...))

The lookup matches any entry with that pubkey, enabled or not. If the file holds a disabled duplicate before the enabled one, the change lands on the disabled entry. seed_overrides then ignores it on restart, and the 202 was a lie. Add definition.enabled && to the predicate, or iterate via enabled().

Relatedly, persist_import matches by raw string (existing.voting_public_key == base), while this path parses. A hand-written file with uppercase hex gets a duplicate entry on import but matches here. Consider sharing one parse-based matcher.

3. Startup reads the file twice (lib.rs ~L157)

ValidatorDefinitions::open is called again after the store has already loaded from it. The two reads can see different contents, and the file is parsed twice. Prefer seeding from the same ValidatorDefinitions instance load used. It's minor, but it also makes the "enabled entries ⇔ loaded keys" relationship explicit rather than coincidental.

Compatibility claim

4. "A file written by either client reads in the other" is likely false (definitions.rs doc, proposer_settings.rs module doc, docs/cli.md)

Lighthouse's ValidatorDefinition flattens SigningDefinition, which is #[serde(tag = "type")]. A file ethlambda writes has no type: local_keystore field, so Lighthouse should reject it. The reverse direction does work, as your test shows.

Two further mismatches:

• Lighthouse accepts a zero suggested_fee_recipient. ethlambda refuses startup on one.
• Lighthouse's graffiti length rules may differ from graffiti_from_text.

Either soften the claim to "reads Lighthouse's file" or emit type: local_keystore.

Performance / locking

5. The store read lock is held across synchronous file I/O (persist_and_apply)

open/save are blocking std::fs calls, likely with fsync via secure_fs, run on the async executor. The store read guard is held throughout. Tokio's RwLock is fair, so a queued writer (a keystore import or delete) blocks the duty loop's subsequent reads behind this fsync.

definitions_lock already serialises against delete, since delete takes it first. So the store guard can probably be dropped right after require_held; verify that delete holds definitions_lock across its whole sequence. Wrapping the open/mutate/save in spawn_blocking would also be better hygiene. This pattern likely pre-exists in the keystore routes, so it's not a blocker.

Tests / build

6. libc dependency (proposer.rs test a_file_that_cannot_be_written_is_a_500...)

The test uses unsafe { libc::geteuid() }, but no Cargo.toml change appears in the diff. Confirm libc is already a (dev-)dependency of ethlambda-validator, or this won't compile. Alternatively, probe writability directly (try creating a file in the chmod'd dir and skip if it succeeds) and avoid unsafe entirely.

Also, if send panics, the directory stays at 0o500 and TempDir cleanup fails noisily. That's acceptable, but a small drop guard that restores permissions would be cleaner.

Nits

• definitions.rs overrides() / seed_overrides: reusing Error::Keystore with a synthetic path string for definitions-file errors is a stretch. A dedicated Error::Definitions { entry, reason } would read better in logs.
• proposer_settings.rs: ADDRESS_BYTES is declared after fee_recipient_from_text, which uses it. Move it up next to GRAFFITI_BYTES for readability.
• The set_*/clear_* wrappers on ProposerSettings are now thin aliases for apply. If nothing outside tests uses them, consider removing them to keep one entry point.

Overall this is close to mergeable. Item 1 and Item 2 are worth fixing first, and the compatibility wording (Item 4) should be corrected.

---

Automated review by Claude · claude-opus-5-5 · custom prompt

[github-actions]

🤖 Codex Code Review

• [P2] Restore retained overrides when re-importing a key — keystores.rs:350. Importing a previously disabled validator preserves its persisted overrides and enables the key, but finish_import only inserts the signing key. Since startup skips disabled entries, its settings remain at defaults until restart, potentially paying execution rewards to the wrong address. The same mismatch occurs after a failed deletion clears settings but leaves the definitions entry. Validate and seed the retained overrides when activating the imported key, and add a regression test for importing a disabled entry.

Tests could not run because Rustup attempted to write to its read-only temporary directory.

---

Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt

[github-actions]

🤖 Gemini Code Review

The changes look solid and well-tested.

Key highlights:

• Atomicity and lock ordering: persist_and_apply in crates/validator/src/http_api/proposer.rs acquires definitions_lock followed by store.read(), matching the lock acquisition order in the keystore routes to prevent deadlocks and ensuring in-memory settings are only mutated after validator_definitions.yml is durably updated on disk.
• Validation on startup: ValidatorDefinition::overrides and ValidatorDefinitions::seed_overrides strictly validate suggested_fee_recipient (including rejecting the zero address) and graffiti byte length on startup, failing fast rather than silently falling back to defaults on malformed configuration.
• Keystore re-imports: persist_import in crates/validator/src/http_api/keystores.rs preserves existing per-validator overrides via with_overrides_of.
• Lighthouse compatibility: Using #[serde(default, skip_serializing_if = "Option::is_none")] with matching field names (suggested_fee_recipient, graffiti, gas_limit) cleanly maintains cross-client file compatibility.

---

Automated review by Gemini · gemini-4-argon · custom prompt

[github-actions]

🤖 Kimi Code Review

No blocking correctness or security issues found. The persist-before-memory ordering in persist_and_apply, consistent lock ordering, zero-address rejection, startup seeding, and preservation of overrides during re-import all look coherent. The tests cover the important restart, failed-write, missing-entry, and deletion paths.

One non-blocking performance consideration:

• crates/validator/src/http_api/proposer.rs, persist_and_apply (~lines 130–170): the handler performs synchronous YAML reads, serialization, and filesystem writes on the async runtime while holding both the definitions lock and store read guard. This is reasonable for low-frequency keymanager requests, but large validator sets or a slow filesystem could stall a runtime worker and delay other validator tasks. If those deployments are expected, consider offloading the persistence critical section to a blocking worker while retaining the current lock and disk-before-memory ordering.

---

Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt

[github-actions]

🤖 DeepSeek Code Review

This PR looks like a solid improvement to validator settings persistence. It doesn’t touch consensus/fork-choice/state-transition code, so the review below focuses on the validator and persistence paths.

Findings

1. Lighthouse file compatibility is not round-trip safe
   In crates/validator/src/keys/definitions.rs, ValidatorDefinition now models the three override fields, but it still does not include Lighthouse-only fields such as type, description, and builder_proposals. Serde ignores those on deserialization, but when ValidatorDefinitions::save is called they will be dropped.

   This matters because this PR now writes validator_definitions.yml on every feerecipient/graffiti/gas_limit change via persist_and_apply in crates/validator/src/http_api/proposer.rs around lines 120-160. Starting from a Lighthouse-written file and setting one override will silently strip those Lighthouse fields. The docs claim “so its file reads here and this one reads there,” but the test a_lighthouse_written_entry_parses_with_its_overrides only checks parsing, not save round-trip.

   Consider either modeling those fields explicitly or using #[serde(flatten)] to preserve unknown fields, and add a test that parses a Lighthouse-style entry, saves, rereads, and confirms the extra Lighthouse fields survive.

2. store read guard is held across blocking file I/O
   In persist_and_apply (crates/validator/src/http_api/proposer.rs around lines 125-155), the tokio RwLock read guard from context.store.read().await is retained while ValidatorDefinitions::open, definitions.save, and context.settings.apply run.

   According to the module’s own lock-order documentation, all keystore-routes take definitions_lock first, so that lock alone prevents a concurrent keystore delete from interleaving with this file read-modify-save. Holding the store read guard across potentially slow disk writes can block writers and, because tokio RwLock is write-preferring, can also delay subsequent readers.

   Suggest dropping the store guard immediately after require_held, performing the file mutation under definitions_lock, then applying the in-memory change. If the guard is intentionally held to block a path that does not take definitions_lock, that path should be documented.

3. Startup validation only covers enabled entries
   seed_overrides in crates/validator/src/keys/definitions.rs iterates self.enabled(), so a malformed fee recipient or graffiti on a disabled entry is silently ignored at startup. The CLI docs now say malformed fields refuse startup, without limiting that statement to enabled entries.

   This is likely acceptable for disabled entries because they are inert, but it would be good to either validate all entries or explicitly document that disabled entries are skipped.

4. Re-import preserves malformed override strings without validation
   In crates/validator/src/http_api/keystores.rs, the re-import path uses:

   let definition = definition.with_overrides_of(&definitions.0[index]);

   with_overrides_of copies the existing override strings without calling ValidatorDefinition::overrides(). If validator_definitions.yml was manually edited to hold a malformed suggested_fee_recipient or graffiti while the client is running, re-import will preserve the malformed value and save it successfully, but the next startup will then fail.

   This is only reachable via unsupported manual editing of a live file, but it may be worth validating the old overrides before carrying them over, to keep the “malformed refuses startup” story consistent.

Overall, the persist-before-apply approach, lock ordering, and tests for failed writes are good. The main concern is the Lighthouse file round-trip behavior.

---

Automated review by DeepSeek · deepseek-v4-pro · custom prompt