[pablodeymo]

Stacked on #734, rebased onto its review follow-ups.

🗒️ Description / Motivation

#734 made slashing protection durable, but history could not move in or out. A staker migrating keys to or from ethlambda had no safe path: a key arriving from another client started as "never signed", and a key leaving got only an empty interchange from DELETE /eth/v1/keystores. EIP-3076's interchange format exists for exactly this, and every other client supports it.

This PR adds EIP-3076 version 5 import and export, offline through a new ethlambda slashing-protection command and online through the keymanager API. It also runs the shared eth-clients/slashing-protection-interchange-tests vectors.

What Changed

• slashing_protection (validator crate):
  • import: parses a complete or minimal interchange and checks the version and genesis_validators_root. It then reduces each validator to its highest slot, source and target, and merges the result with the existing record by maximum, in one durable write.
    • Merging by maximum never lowers a watermark.
    • Slashable data in the file is absorbed rather than refused, as Lighthouse and Nimbus do, except an attestation whose source is at or past its target outside genesis, which fails the whole import: it is never valid, and read into the record it would refuse every honest attestation until the chain justified past that source.
    • All or nothing.
  • export: writes the record, or selected keys, as a minimal interchange. Keys with no record are omitted.
  • open_offline: opens the record without a beacon node, creating one for a given chain if there is none.
  • interchange_chain, contains, pubkeys, genesis_validators_root.
  • file.rs: parsing is split from reading. An import must accept a pubkey listed twice, and an attestation whose source is after its target; the vectors require both. The client's own record still refuses a duplicate pubkey.
• ethlambda slashing-protection import <file> / export [--pubkeys ...] <file> (bin/ethlambda/src/slashing_protection.rs):
  • offline, against --validators-dir;
  • takes the validator client's lock, so it refuses while a client runs there;
  • export refuses to overwrite an existing file.
• Keymanager API:
  • POST /eth/v1/keystores judges slashing_protection before touching anything: if it does not parse or is for another chain, every key in the request fails and none is loaded. It is written before any key is saved, so a crash between saving a key and registering it cannot load a returning key on the record it had before it left; the history only ever raises watermarks, so a key that then fails to save leaves nothing dangerous behind. The keys are registered after they are saved, under the definitions lock, so a concurrent DELETE cannot interleave.
  • DELETE /eth/v1/keystores returns the deleted keys' real records, replacing the deliberately empty, all-zero-root interchange.
    • The export is taken after the keys leave the store, so it covers every signature made with them.
    • A key that is gone but still has a record answers not_active.
• Interop vectors:
  • make slashing-protection-interchange-tests downloads v5.3.0. It is pinned, its SHA-256 is checked, and the download is stamped by version. make test-node depends on it.
  • crates/validator/tests/interchange_vectors.rs runs all 38 cases against the should_succeed (minimal) expectations, as Teku, Nimbus and Lodestar do.
• Docs: docs/cli.md gets a new slashing-protection section and the keymanager notes; docs/spec_deviations.md, README.md and the crate docs are updated.

Correctness / Behavior Guarantees

• An import never makes the record less strict: every field is merged by maximum, including against existing records and repeated entries in the same file.
• The chain must match: an interchange for another chain is refused by the record and by the keymanager. The offline import into an empty directory creates the record for the file's chain, and the client refuses it at its next start if that is not its beacon node's chain.
• Keymanager ordering: on POST, the history is judged and written before any key is saved, so no key ever signs, or loads after a crash, ahead of its history. On DELETE, the keys leave the store before the export. If the export fails, only the keys actually deleted become errors; not_found and malformed pubkeys keep their statuses.
• Behavior change: DELETE /eth/v1/keystores now returns real history with the real genesis_validators_root, where it used to return an empty one with an all-zero root.

Tests Added / Run

• The 38 EIP-3076 v5.3.0 vectors, all passing. They cover 150 signing attempts, 113 of which must be refused.
  • Mutation check: with the double-vote rule weakened (<= to <), 10 of the 38 fail.
• Unit tests:
  • import raises the watermarks and survives a restart;
  • import never lowers a watermark;
  • another chain changes nothing;
  • export lists only keys with a record and round-trips into a fresh record;
  • open_offline needs a record or a chain.
• Keymanager tests:
  • an import brings its history;
  • history for another chain imports no key;
  • delete returns the key's history, then not_active.
• CLI: a parse test for slashing-protection export. Smoke-tested by hand: import, export, a refused overwrite, and a missing record.
• Commands run:
  • make fmt: clean
  • make lint: clean
  • make test: 2888 passed, 0 failed (after the review follow-ups)

Related Issues / PRs

• Stacked on feat(validator): durable EIP-3076 slashing protection #734 (durable slashing protection).
• Follow-up: doppelganger protection.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — all passing

[github-actions]

🤖 Kimi Code Review

I'll review this PR implementing EIP-3076 slashing protection interchange import/export functionality. Let me analyze the code systematically.

Overall Assessment

This is a well-structured PR adding slashing protection interchange support. The design is sound with proper "history first, keys second" semantics, offline locking, and good test coverage. I found several issues ranging from minor to moderate severity.

---

Security & Correctness Issues

1. TOCTOU in export file creation (bin/ethlambda/src/slashing_protection.rs:88-93)

let mut output = std::fs::OpenOptions::new()
    .write(true)
    .create_new(true)
    .open(&file)

Issue: create_new prevents overwrite, but there's a race between checking and opening. More critically, the file is created with default permissions (world-readable on many systems), which could expose sensitive slashing protection data.

Fix: Set restrictive permissions before writing:

#[cfg(unix)]
{
    use std::os::unix::fs::OpenOptionsExt;
    options.mode(0o600);
}

2. Missing fsync on parent directory (bin/ethlambda/src/slashing_protection.rs:92-93)

output.write_all(interchange.as_bytes())
    .and_then(|()| output.sync_all())

Issue: sync_all fsyncs the file but not the parent directory. If the system crashes after fsync but before directory entry is durable, the file may be lost. The file.rs module's write_durably does this correctly—this code bypasses it.

Fix: Use file::write_durably or replicate its directory-fsync logic.

3. Lock poisoning handling (crates/validator/src/http_api/keystores.rs:170-172, 440-442)

.map_err(|_| "slashing protection is poisoned".to_string())

Issue: Lock poisoning is treated as fatal without recovery attempt. In a validator client, this could require restart. The offline CLI tool (slashing_protection.rs) doesn't use Mutex at all—good—but the HTTP API paths do.

Note: This is pre-existing pattern, but worth noting. Consider into_inner() recovery if poisoned data is acceptable.

---

Consensus & Logic Issues

4. not_active status logic may misclassify (crates/validator/src/http_api/keystores.rs:474-485)

if status.status == "not_found" && recorded {
    KeyStatus {
        status: "not_active".to_string(),
        message: None,
    }
} else {
    status
}

Issue: A key that was error (e.g., lock poisoned during delete_one) but happens to have a record gets its error status overwritten with not_active, hiding the actual failure.

Fix: Only override not_found, not other error statuses:

if status.status == "not_found" && recorded {
    // ...
} else {
    status  // preserves errors
}

Wait—re-reading: delete_one returns KeyStatus with status "deleted" or "error". The results tuple is (Option<BlsPubkey>, KeyStatus). If pubkey is None, it was parse error. If pubkey is Some but status is "error", that's from delete_one. The recorded check uses pubkey.is_some_and(...), so recorded is false for parse errors (pubkey is None). For delete_one errors, recorded could be true if the record still exists.

Actually, looking more carefully: delete_one errors include persistence failures. The key was in the store, deletion may have partially succeeded. Returning not_active instead of error seems wrong—caller may think deletion+export succeeded.

Line 474-485: The condition should be more precise:

if status.status == "not_found" && recorded {
    // key not in store but has record → not_active
} else {
    status  // keep "deleted", "error", etc.
}

This is actually what the code does. But what if status.status == "error" and recorded? Then we return status unchanged, which is correct. My initial concern was wrong—the code is correct here.

5. Import ordering in prepare_import vs import (crates/validator/src/http_api/keystores.rs)

The import function imports slashing protection before calling prepare_import, which then calls register_new_keys. But register_new_keys is also called for keys that already have records (from previous import or signing). The comment says "A key that already has a record keeps it, including one the request's own history just created." This is correct.

However, there's a subtle issue: if the interchange contains a validator with no history (empty watermarks), and that validator is also in the request's keystores, register_new_keys will create a "never signed" record. But the interchange might have intended to convey "this validator exists but we have no history" vs "this validator has signed nothing." These are semantically different but treated the same.

This is acceptable per EIP-3076 minimal strategy, but should be documented. The current behavior is safe (conservative).

---

Performance & Resource Issues

6. String allocation in hot path (crates/validator/src/slashing_protection/file.rs:110-113)

pub(crate) fn parse(text: &str) -> std::result::Result<(Root, Vec<(BlsPubkey, Record)>), String> {

Issue: Returns Vec<(BlsPubkey, Record)> which allocates. For large interchanges (thousands of validators), this is fine. But import then builds a HashMap from it. Could stream, but complexity may not be worth it.

Minor: The String error type forces allocation on every error path. Consider &'static str for static messages.

7. pubkeys() allocates unnecessarily (crates/validator/src/slashing_protection/mod.rs:483-485)

pub fn pubkeys(&self) -> Vec<BlsPubkey> {
    self.records.keys().copied().collect()
}

Used in slashing_protection.rs:79 when --pubkeys is empty. For large validator sets, this clones all keys. An iterator-returning API would be more flexible.

---

Rust Best Practices

8. Unnecessary format! in error path (crates/validator/src/slashing_protection/mod.rs:507-511)

return Err(refused(format!(
    "it is for genesis_validators_root {}, but this record is for {}",
    encode_hex(&root.0),
    encode_hex(&self.genesis_validators_root.0)
)));

Issue: encode_hex returns String, so this allocates two strings plus the format string. Use structured logging fields instead, but for error messages this is acceptable.

9. unwrap_or_default() for Record (crates/validator/src/slashing_protection/mod.rs:519-520)

.copied()
.unwrap_or_default()

Record::default() is #[derive(Default)] which gives block_slot: None, attestation: None. This is correct—"never signed" is the safe default. But explicit would be clearer:

.unwrap_or(Record { block_slot: None, attestation: None })

10. Test helper interchange uses format! with raw JSON (crates/validator/src/slashing_protection/mod.rs:967-972)

format!(
    r#"{{"metadata":...}}"#
)

Issue: Brittle—field order matters for tests comparing JSON strings. The render function sorts by pubkey, but this raw string doesn't. Tests use serde_json::from_str so order doesn't matter for parsing, but string comparison would fail.

Actually, looking at usage: tests parse with serde_json, so this is fine. But consider using serde_json::json! macro for type safety.

---

Error Handling & Edge Cases

11. Empty --pubkeys list behavior (bin/ethlambda/src/slashing_protection.rs:75-79)

let pubkeys = if pubkeys.is_empty() {
    protection.pubkeys()
} else {
    // parse provided
};

Issue: If user passes --pubkeys "" (empty string), value_delimiter = ',' produces vec![""], which fails in parse_pubkey. This is correct behavior, but error message could be clearer.

12. Export with non-existent pubkeys silently excludes them (crates/validator/src/slashing_protection/mod.rs:536-542)

let selected = pubkeys
    .iter()
    .filter_map(|pubkey| self.records.get_key_value(pubkey));

Issue: No warning that some requested pubkeys were skipped. User might think export succeeded for all requested keys.

Fix: Consider returning which pubkeys were missing, or at least logging.

13. parse_pubkey error propagation (bin/ethlambda/src/slashing_protection.rs:80-82)

pubkeys
    .iter()
    .map(|text| parse_pubkey(text))
    .collect::<Result<Vec<_>, _>>()?

Issue: parse_pubkey error doesn't indicate which index failed. For --pubkeys 0xaa,0xbb,0xcc, if 0xbb is invalid, user just gets parse error without position.

---

Documentation & Comments

14. Comment typo (crates/validator/src/slashing_protection/mod.rs:316-317)

/// Open the record in `validators_dir` without a beacon node, for the
/// `slashing-protection` command, holding its lock until this is dropped.

"until this is dropped" → "until this SlashingProtection is dropped" for clarity.

15. README update is slightly misleading (README.md:340-343)

A key moving from another client brings its history with ethlambda slashing-protection import (or the keymanager API)

The keymanager API import is for POST /eth/v1/keystores which imports keys and history together. The slashing-protection command is offline. The phrasing is slightly ambiguous about whether import is keymanager or slashing-protection.

---

Test Issues

16. Test vector runner doesn't validate contains_slashable_data (crates/validator/tests/interchange_vectors.rs:160-180)

// The README lets an importer refuse an interchange holding
// slashable data, in which case the rest of the case is skipped.
// This one absorbs it instead, so reaching here is still a
// failure: it would mean an import was refused for some other
// reason.

Issue: The comment says this client absorbs slashable data, but the test doesn't verify that contains_slashable_data: true cases actually have their subsequent operations succeed. It only checks import success/failure. If import succeeds but later blocks/attestations fail unexpectedly, the test catches it, which is correct.

However, the test could be more explicit: when contains_slashable_data: true and should_succeed: true, verify that the imported data is indeed absorbed (watermarks raised appropriately).

17. Missing test for import idempotency (crates/validator/src/slashing_protection/mod.rs)

No test verifies that importing the same interchange twice is equivalent to importing once. This is important for the "never lowers" guarantee.

18. Missing test for empty interchange rejection (crates/validator/tests/interchange_vectors.rs)

The file::parse function rejects empty strings, but this isn't tested in the vector runner since vectors are non-empty.

---

Minor Code Quality

19. use std::io::Write as _ pattern (bin/ethlambda/src/slashing_protection.rs:9)

use std::io::Write as _;

The as _ is unnecessary—Write isn't ambiguous here. But it's idiomatic to avoid unused import warnings when only the trait methods are used.

20. Inconsistent error formatting (crates/validator/src/http_api/keystores.rs:185-186)

warn!(%err, "Refusing an import whose slashing-protection history was refused");
let message = format!("slashing_protection was not imported: {err}");

The warn! uses structured field %err, but the message uses Display. Consistent, but could use tracing's error reporting more uniformly.

---

Summary Table

Item	Severity	File:Line	Issue
1	Moderate	slashing_protection.rs:88	World-readable export file
2	Moderate	slashing_protection.rs:92	Missing directory fsync
3	Low	keystores.rs:170,440	Lock poisoning not recovered
4	Incorrect analysis	—	not_active logic is actually correct
5	Informational	keystores.rs	Empty history semantic gap
6	Low	file.rs:110	Vec allocation for large imports
7	Low	mod.rs:483	pubkeys() allocates
8	Nit	mod.rs:507	Extra string allocation
9	Nit	mod.rs:519	Implicit default
10	Nit	mod.rs:967	Raw JSON in test
11	Low	slashing_protection.rs:75	Empty string in pubkeys
12	Low	mod.rs:536	Silent exclusion in export
13	Low	slashing_protection.rs:80	No position in parse error
14	Nit	mod.rs:316	Comment clarity
15	Nit	README.md:340	Ambiguous phrasing
16	Informational	interchange_vectors.rs:160	Could verify absorption
17	Low	mod.rs (tests)	Missing idempotency test
18	Low	interchange_vectors.rs	Missing empty interchange test
19	Nit	slashing_protection.rs:9	Style
20	Nit	keystores.rs:185	Logging style

---

Recommended Fixes (Priority Order)

1. File permissions on export (Item 1): Add 0o600 mode on Unix
2. Directory fsync (Item 2): Use write_durably or add sync_dir
3. Add idempotency test (Item 17): Verify double-import is safe
4. Warn on missing pubkeys in export (Item 12): Don't silently skip

The core logic—import ordering, watermark merging, chain validation, and test vector compliance—is correct and well-tested.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: EIP-3076 import/export for the validator client (PR 736)

I read the diff and did not run it. I only saw the first ~1100 diff lines: the CLI, keymanager, file.rs and mod.rs. I skimmed the Makefile and docs, and did not see the tests past the mod.rs unit tests or the test-runner changes. The logic that matters holds up.

Done well

• import stages everything and commits in one durable write. It merges by taking the maximum of each watermark, so an import can never lower protection. Slashable data in the file is absorbed instead of refused, which is what the interchange spec expects of an importer.
• The keymanager POST imports history before any key and fails the whole request if the history is refused. Keys never start signing ahead of their history.
• The DELETE export happens after the store write lock is released. The ordering comment is correct: any signature made with a deleted key is either already in the record or can no longer happen.
• A key with no history is registered as "never signed" with a warning. This is the safe default for a client that does not keep complete history.
• parse is shared by read, import and interchange_chain. Empty input, a bad version and a chain mismatch are all rejected, and none of them changes anything.
• Removing the all-zero genesis_validators_root hack is correct now that the real record is exported.

Points to consider

1. file.rs record_of no longer rejects source > target, and read shares that path. Tolerating it on import is right. But a corrupt or hand-edited on-disk record with source > target is now accepted silently at startup, where it used to be refused. The result is still conservative, so this is not a safety problem. Splitting the strictness (lenient for import, strict for read) would keep the old corruption check, or you could note the relaxation explicitly.
2. slashing_protection.rs export logs validators = pubkeys.len(). That is the number requested. export silently drops keys that have no record, so the log can overstate what was written. Consider returning the count actually exported, or warning about the omitted keys. For a migration tool, a silent omission is easy to miss.
3. open_offline(.., Some(chain)) creates an empty record file before import runs. If the commit then fails (I/O), a record for that chain is left behind, and open will refuse it if the chain was wrong. The doc comment covers the wrong-chain case. Writing the file only once the import succeeds would be cleaner.
4. Export uses create_new and then write_all. If the write fails partway, a truncated file is left and blocks a retry because the file now exists. Consider removing it on error, or writing to a temp file and renaming. A truncated JSON fails to parse on import, so this is not unsafe.
5. import in keystores.rs calls protection.import, which does an fsync, while holding a std::sync::Mutex inside an async handler. The existing registration path already works this way, so I am not asking for a change. If you want it off the executor, use spawn_blocking.
6. The interchange may name pubkeys that are not in the request's keystores. Those get records anyway, and a later DELETE for them returns not_active. That is spec-compatible, but it is worth a line in the docs.

Security/consensus
I found nothing blocking. The paths are fail-closed (all or nothing, chain check, maximum merge). The offline command takes the client's lock, so it cannot race a running validator.

Overall this looks good to merge. Points 2 and 3 are the ones I would fix first.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

I found two review points; the rest looks solid.

• crates/validator/src/http_api/keystores.rs:166 — Correctness / atomicity bug: slashing_protection is imported before definitions_lock is taken and before any keystore is validated/persisted. If the subsequent keystore import fails (bad password, malformed keystore, filesystem error, duplicate path, etc.), the request returns errors but the slashing-protection record has already been durably mutated. That breaks the “all or nothing” behavior at the request level and can accumulate orphaned records for validators that were never actually imported. More seriously, a concurrent DELETE can interleave between the history import and the later key persistence because DELETE only serializes on definitions_lock, not on the protection mutex. I’d move the history import under the same request-wide critical section as definitions mutation, or stage it and commit only after the keystore batch is known-good.

• crates/validator/src/http_api/keystores.rs:455 — API semantics concern: on export failure, the handler rewrites every result to error, including malformed pubkeys that were already correctly classified earlier. That makes per-key status less trustworthy and hides which inputs were invalid versus which deletions succeeded but could not be exported. Consider preserving the original per-key statuses and only attaching a request-level failure in slashing_protection/message handling.

• crates/validator/src/slashing_protection/mod.rs:501 — The import merge logic looks conservative in the right direction: taking maxima over block slot and (source,target) watermarks preserves safety and intentionally prefers false positives over false negatives. Good choice for slashing protection.

• crates/validator/src/http_api/keystores.rs:439 — Exporting only after the store write lock is acquired and the key is removed is the right ordering for “no more signatures can happen after this export.” That part looks carefully reasoned.

• bin/ethlambda/src/slashing_protection.rs:82 — Nice security/property choice to use create_new(true) for export output; it avoids silently overwriting an existing interchange file.

If you want, I can turn Item 1 into a concrete fix sketch.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[MegaRedHand]

Re-reviewed at 721c300, after the rebase onto #734. I compared the keymanager and interchange behavior with Lighthouse, Teku, Prysm, Nimbus and Lodestar; where another client's behavior is the reason for a suggestion, the comment says so. Comments inline.

[MegaRedHand]

The keymanager router sets no DefaultBodyLimit, so POST /eth/v1/keystores gets axum 0.8's 2 MB default for Json. Now that the request carries slashing_protection, a complete-format interchange for a batch of validators can exceed that, and the whole import fails with 413.

For comparison: Nimbus allows 16 MiB and Lodestar 20 MB (both configurable), and Lighthouse and Prysm set no limit. Could we set DefaultBodyLimit::max(64 * 1024 * 1024) on the keymanager router, or at least on this route?

[pablodeymo]

Done in cd5e3df: a DefaultBodyLimit::max(64 MiB) layer on the keymanager router, with the other clients' numbers in the comment.

[MegaRedHand]

Could the history and the registration go in one write? Today import commits the history, then register commits again. If the second write fails, every persisted key reports "saved, but not loaded … refused at the next start until it has a record". But the keys the history covers already have a record, so they load and sign after a restart. Staging both and committing once makes that message true. Combined with the rollback I suggested on #734 (undo persist_import when registration fails), a failure then leaves nothing behind.

It also lets the interchange be parsed once. Today check_import (slashing_protection/mod.rs:587) parses it and repeats import's chain check, then import parses it again, both under the mutex. Parsing once outside the lock into staged records, then committing staged history plus new registrations under it, removes both.

Related: the module doc (line 12) says "a key that failed to import leaves none behind", but the whole interchange is imported, including entries for request keys that failed. Either leave those entries out of the staged write or adjust the doc.

[pablodeymo]

Done in cd5e3df. The interchange is parsed once, before any lock, into a StagedHistory, judged against the record's chain, and then merged and the keys registered in one durable write through a new SlashingProtection::absorb; import and register are now wrappers over it and check_import is gone. A failed write holds neither, so the "refused at the next start" message is true again, and combined with the rollback on #734 nothing is left behind. a_failed_write_holds_neither_history_nor_registration pins it.

On the module doc: I went with your second option. The history is written whole, entries for request keys that failed included, since real history is never the dangerous direction and a later import of such a key merges with it; the doc says so now. A request that saved no key still writes nothing, so history_whose_key_failed_to_import_is_not_written stands.

[MegaRedHand]

The keymanager spec says "Slashing protection data must only be returned for keys from request.pubkeys for which a deleted or not_active status is returned" (apis/local_keystores.yaml). This exports every parsed pubkey, including ones whose delete_one returned error. For example, when persist_delete failed, the key is still in the definitions file and comes back on restart, yet its history is handed to the caller.

Teku and Prysm filter on the status. Exporting only the keys whose delete_one status is deleted or not_found would match them: not_found keys become not_active when they have a record, and export already skips keys without one.

[pablodeymo]

Done in cd5e3df: the export is built only from keys whose status is deleted or not_found (the latter becoming not_active when recorded), so a key whose delete failed keeps its history here, matching Teku and Prysm.

[MegaRedHand]

When the export fails, recorded is never computed, so nothing here can tell a key with no record from one whose record could not be read. Mapping only deleted to error means a key that is gone but has a record answers not_found. That tells a migrator there is no history to carry, and the comment above says such a key "becomes an error".

Prysm marks every key error in this case. Doing the same here, for every parsed pubkey, would be the safe fallback.

[pablodeymo]

Done in cd5e3df: when the export fails, every key that parsed is an error, as Prysm answers; only a malformed pubkey keeps its own error.

[MegaRedHand]

Nit: "stays protected" is a bit strong. The kept record covers what this client signed, not what the key signed elsewhere in between. That is the same caveat spec_deviations.md makes about doppelganger protection.

[pablodeymo]

Reworded in cd5e3df: importing it back later starts from what this client signed with it, with a pointer to the doppelganger caveat in spec_deviations.md.

[MegaRedHand]

When a requested key has no record, this only warns and still exits 0, possibly writing an interchange with no entries. So export --pubkeys K out.json && import elsewhere succeeds with nothing to import, for example after a typo or the wrong --validators-dir.

Lighthouse fails the export instead when a --pubkeys key has no record (error, exit 1, no file). Could we do the same?

[pablodeymo]

Done in cd5e3df: a requested validator with no record fails the export (exit 1, no file, naming the count and the first key), as Lighthouse does. docs/cli.md says so.

[MegaRedHand]

This hard-fails when the vectors have not been downloaded, so a plain cargo test --workspace (CLAUDE.md's "all workspace tests") now fails on a fresh checkout. The other fixture suites are test = false in their Cargo.toml and run explicitly (--test <name>), or sit behind required-features. Could this one follow the same pattern, with test-node running it explicitly?

[pablodeymo]

Done in cd5e3df: interchange_vectors is a [[test]] with test = false, and make test-node and make lint run it by name, like the other fixture suites. A plain cargo test --workspace no longer needs the download.

[MegaRedHand]

Nit: ethlambda_validator::beacon_node::dto::parse_root is public and does this. parse_root(text).map_err(|err| err.to_string()) would do, like parse_pubkey above.

[pablodeymo]

Done in cd5e3df.

[MegaRedHand]

Nit: file::lock creates slashing_protection.lock before anything else, so a failed import does not quite leave the directory as it found it (docs/cli.md also says "a failed import creates nothing"), and export needs a writable directory. Keeping the lock is fine; it is only the wording of these docs.

[pablodeymo]

Docs fixed in cd5e3df: a failed import creates nothing but the lock file, and export takes the lock, so the directory must be writable. The lock stays.

[pablodeymo]

/review

[github-actions]

🤖 Codex Code Review

• [P1] Persist imported history before enabling keys on disk — keystores.rs:260. prepare_import saves an enabled validator definition before absorb persists its supplied history. If a previously deleted validator returns after signing elsewhere, its old local record still exists. A crash between these writes lets startup load the key using stale watermarks, potentially signing a slashable message. The missing-record startup check does not protect this case. Keep imported definitions disabled until history is durable, or use a durable pending-import marker that blocks startup. Add a restart regression test for this sequence.

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

[github-actions]

🤖 Claude Code Review

Review: EIP-3076 interchange import/export (PR 736)

The design holds up. Merge-by-maximum is the right semantics for minimal watermarks. Re-checking the chain inside absorb closes the race between the pre-lock check_staged and the write. Exporting on DELETE after the store write lock is a good ordering choice. Running the shared EIP-3076 vectors adds real confidence.

Correctness

1. Duplicate pubkeys produce duplicate interchange entries (slashing_protection/mod.rs, export).

   • export(&[pk, pk]) maps through get_key_value, and render sorts but does not deduplicate. Both callers can pass duplicates:
     • CLI: --pubkeys 0xab..,0xab...
     • DELETE: a request listing the same key twice gets deleted, then not_found. Both land in requested, so the key is exported twice.
   • This client's own parse merges duplicates, but other importers may refuse the file. This client's read also rejects duplicates in a record file.
   • Fix: dedupe in export, e.g. collect into a BTreeSet/HashSet first.
   • The same applies to the exported = pubkeys.len() count in bin/ethlambda/src/slashing_protection.rs.

2. DELETE export race depends on how the duty path holds keys (keystores.rs, the "Exported only now" comment).

   • The claim "a signature with one of them either finished before the write lock above was granted … or can no longer happen" holds only if the duty path does both of these while still holding the store read guard:
     • calls check_and_record_*, and
     • signs.
   • If the duty path clones the keypair out of the store and releases the guard first, it can record and sign after the export was taken. The handed-over history would then be stale, which is exactly the migration slashing scenario.
   • Please confirm the invariant, and ideally document it next to the duty-path code that relies on it.

3. Export failure on DELETE misreports deletions (keystores.rs, the Err(err) branch).

   • Keys that were actually removed from disk are reported as error.
   • slashing_protection: String::new() is not a valid interchange. The spec makes it a required, well-formed field.
   • A retry recovers this (the key becomes not_active with history), so it is acceptable. Still, consider a comment noting the deletions already happened, or at least logging which keys were deleted.

Security

4. POST history can raise watermarks of currently active keys, irreversibly (absorb).

   • The history may name validators outside the request, which is documented. Those include keys currently signing.
   • An interchange with slot: u64::MAX for an active key bricks it in this record permanently. Nothing lowers a watermark short of hand-editing the file.
   • This requires the bearer token, and Lighthouse behaves the same. Two cheap safeguards to consider:
     • warn when imported history touches a key already loaded in the store, or
     • restrict POST history to the request's keys plus keys with no record.

5. Body limit of 64 MiB (http_api/mod.rs). This sits above the Nimbus (16 MiB) and Lodestar (20 MB) limits that the comment cites. It is fine because DefaultBodyLimit only affects extractors, which run after require_bearer. A short note to that effect would keep a future reordering of layers from silently exposing it.

Build / tooling

6. Makefile portability and pinning.

   • sha256sum is absent on stock macOS; consider falling back to shasum -a 256.
   • Pinning the SHA-256 of a GitHub auto-generated tag archive is fragile. GitHub changed archive compression in January 2023, which broke checksums repo-wide before it was reverted. Pinning a commit and verifying the extracted tree, or vendoring the vectors, would be sturdier.
   • make test-node now needs network access. Please note that in CI docs.

7. Stale doc comment (tests/interchange_vectors.rs, header). It says "make test runs first", but only test-node depends on the download target.

Minor / idiomatic

8. Double parse in CLI import (bin/ethlambda/src/slashing_protection.rs, Import arm).

   • interchange_chain fully parses the file, then import parses it again. For large complete-format files this doubles the work.
   • Suggested flow:
     1. stage(&text)
     2. open_offline(dir, Some(staged.root())) (needs a root() accessor on StagedHistory)
     3. absorb(Some(&staged), &[])
   • With that, interchange_chain can go.

9. Stringly-typed status matching (keystores.rs, "deleted" | "not_found" and status.status == "not_found"). Typos here fail silently; an enum would make them compile errors.

10. record_of no longer validates the client's own record file.

    • Dropping the source > target rejection is correct for imports, and is required now that merged watermarks can legitimately have source > target.
    • It does mean read no longer catches that class of corruption in the record itself. It also lost the per-validator context in errors.
    • Acceptable, since the result is never less strict, but worth a one-line comment in read.

11. write_export cleanup (bin/ethlambda/src/slashing_protection.rs). It is correct because create_new guarantees the file is ours. A rename-from-temp approach would avoid even briefly exposing a partial file at the destination; optional.

Tests

Good coverage overall: round-trip, idempotence, never-lower, wrong chain, failed persist, and failed record write. Two cases are missing:

• Duplicate pubkeys: a DELETE test with the same key twice, which would catch Item 1.
• POST history for a key not in the request: a test asserting that such a key gets a record.

---

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

[github-actions]

🤖 Gemini Code Review

The implementation is well-structured, with thorough test coverage (including the EIP-3076 v5.3.0 test vectors) and careful ordering of lock acquisition and durable writes in the keymanager API.

A couple of suggestions below:

1. Duplicate pubkeys in export produce duplicate entries in the exported interchange JSON (crates/validator/src/slashing_protection/mod.rs:629-639, crates/validator/src/slashing_protection/file.rs:209-236, bin/ethlambda/src/slashing_protection.rs:73-101)
   SlashingProtection::export iterates over pubkeys directly without deduplicating:

   let selected = pubkeys
       .iter()
       .filter_map(|pubkey| self.records.get_key_value(pubkey));
   file::render(self.genesis_validators_root, selected)

   If duplicate pubkeys are passed via ethlambda slashing-protection export --pubkeys 0xaa,0xaa or in a DELETE /eth/v1/keystores request body ({"pubkeys": ["0xaa...", "0xaa..."]}, where the first yields "deleted" and the second yields "not_found" -> "not_active", both kept in requested), file::render will emit multiple entries for the same pubkey in data.
   Since file::render already sorts data by pubkey at crates/validator/src/slashing_protection/file.rs:236, adding data.dedup_by(|left, right| left.pubkey == right.pubkey); (and/or deduplicating pubkeys in the CLI so the logged validators = exported count is accurate) ensures each validator is emitted at most once.

2. Avoid parsing the interchange JSON twice during CLI import (bin/ethlambda/src/slashing_protection.rs:58-60)
   In Action::Import:

   let chain = SlashingProtection::interchange_chain(&interchange)?;
   let mut protection = SlashingProtection::open_offline(&validators_dir, Some(chain))?;
   let validators = protection.import(&interchange)?;

   interchange_chain and import both run file::parse(&interchange). Since complete-format interchange files from other clients can be tens of megabytes, staging once via SlashingProtection::stage(&interchange) before open_offline (exposing the root on StagedHistory and passing &staged to absorb) would avoid deserializing the file twice.

---

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

[github-actions]

🤖 Grok Code Review

The import/export design is sound in the ways that matter most: chain mismatch fails closed, watermarks only move by maximum, the validator lock is held for offline import, and create_new plus 0o600 avoids clobbering an export. A few slashing-safety and liveness gaps are still worth fixing.

1. Re-import of a deleted key does not warn, and will sign from stale watermarks. DELETE keeps the record (correct), but absorb only counts a key as "no history" when it has no record at all (slashing_protection/mod.rs, the register loop in absorb). A later POST /eth/v1/keystores without slashing_protection therefore loads the key with no warning, even if it signed elsewhere after the export. That is the slash case the module docs point at. Warn, or refuse, when a persisted key already has a record and this request did not carry history for it.

2. A failed absorb can leave keystores on disk with no record (keystores.rs, after the blocking(... absorb ...) call). Startup then refuses them, and --init-slashing-protection registers those keys as never-signed, which drops the history the failed import was supposed to install. Roll back the keystores written in this request if absorb fails, or the next "fix startup" flag is a slash footgun. The new test only checks the in-memory record.

3. No test shows that an import raises an existing watermark. an_import_never_lowers_a_watermark still passes if commit ignores updates to keys already in the map (the old register path only ever inserted new keys). Add: record slot 10, import slot 50, assert 40 is refused and 51 is allowed, including after reopen.

4. DELETE removes keys before export (keystores.rs, delete). If export then fails, the response is all errors plus slashing_protection: "", and the keystores are already gone. Retry via not_active works only if the caller tries again. Export under the same protection lock before unlinking keystores, after the store write lock has excluded new signatures, so a failed encode cannot strand the migration.

5. Confirm the duty path holds the validator-store lock across both the slashing check and the watermark write. The new export comment depends on that. If signing drops the store lock before check_and_record_*, a proposal can land in the record after this export and the migrated client will not know about it.

6. SlashingProtection::stage runs on the async worker (keystores.rs, before definitions_lock), and the body limit is now 64 MiB (http_api/mod.rs). A large interchange stalls attestation duties. Parse inside blocking / spawn_blocking. Same concern after import: this record is rewritten on every sign, and absorb commits every pubkey in the file, not just the keys in the request. A full-network interchange makes every later duty fsync that whole file. For the keymanager path, merge only pubkeys being imported unless you really want the "history for keys not in this request" behavior on the hot path.

7. crates/validator/tests/interchange_vectors.rs imports H256 and never uses it. make lint now clippy-checks this binary with -D warnings, so that will fail. Also, on a step with should_succeed: false, the runner still applies that step's blocks and attestations; skip the rest of the step when import is refused, or the official vectors will drift from what they mean.

Minor: write_export (bin/ethlambda/src/slashing_protection.rs around the sync_all path) does not fsync the parent directory, so a crash can lose the directory entry after a successful return. sha256sum in the Makefile is not portable to macOS.

---

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

[github-actions]

🤖 Kimi Code Review

1. Duplicate validators can be emitted in exports — crates/validator/src/slashing_protection/mod.rs, SlashingProtection::export. The requested pubkeys iterator is not deduplicated. Repeated --pubkeys arguments or duplicate pubkeys in a DELETE request therefore produce duplicate interchange entries, contradicting the invariant documented in file::parse that records written by this client never contain a pubkey twice. Strict importers may reject these exports. Deduplicate centrally in export and add a repeated-pubkey test.

2. Large interchanges are parsed synchronously in the async handler — crates/validator/src/http_api/keystores.rs, around lines 179–184. SlashingProtection::stage performs JSON parsing and allocation directly on a Tokio worker, while http_api/mod.rs now permits bodies up to 64 MiB. A large import can stall that worker and potentially delay validator duties. Move staging to tokio::task::spawn_blocking; it can remain outside the protection and definitions locks.

3. The new lint target has an unused import — crates/validator/tests/interchange_vectors.rs:20: H256 is unused. Since make lint now clippy-checks this target with -D warnings, this will fail lint. Remove H256.

The maximum-based merge, chain validation, and single-write history/registration approach otherwise look appropriately conservative for slashing protection.

---

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

[github-actions]

🤖 DeepSeek Code Review

Reviewed the slashing-protection import/export, keymanager API changes, and vector harness. The core design is fail-closed and the EIP-3076 test wiring is a good addition. A few issues worth addressing:

• Duplicate pubkeys produce an invalid interchange. In keystores.rs delete, requested is built from every result whose status is deleted or not_found. If a request contains the same pubkey twice, the first delete produces deleted and the second not_found; both are exported. SlashingProtection::export then iterates the supplied pubkeys slice verbatim, so render emits two entries for the same pubkey. file::read itself rejects duplicate pubkeys, so this file cannot be imported back into this client. The same applies to the CLI when --pubkeys repeats a key. Dedup the export list or reject duplicate --pubkeys/request pubkeys.

• POST /eth/v1/keystores can raise watermarks for validators not in the request. absorb merges every entry in the supplied interchange, including pubkeys that are neither persisted in this request nor even present in the validator store. That is documented, but it means a caller with keymanager access can poison active/passive keys’ slashing-protection watermarks without having their keystores. Depending on the API’s trust model, consider restricting absorbed history to keys being imported, or at least excluding already-active keys.

• Failed absorb after key persistence may leave keystores on disk without records. The new test a_failed_write_holds_neither_history_nor_registration checks that in-memory protection is empty, but not that the keystore file/definitions were rolled back. If persist_import has already written a keystore and absorb later fails, the import returns an error but the key may remain on disk. The next startup will then refuse that key because it has no record. If the handler does not already roll back persisted keystores on registration/history-write failure, add that rollback.

• DELETE reports an error after the key has already been deleted if export fails. In the export-error path, the keystore has already been removed, but all parsed pubkeys are converted to KeyStatus::error. That can mislead a caller into thinking the key is still present. The record is kept and a retry may recover the history, but the response should make the state clearer, e.g. "deleted, but slashing_protection could not be exported" rather than a generic error. The empty slashing_protection: String::new() on that path is also not a valid interchange field.

• The up-to-64 MiB interchange parse runs directly on the async handler. SlashingProtection::stage is called before the protection lock/blocking task, so a large serde_json::from_str can block a Tokio worker. Consider moving the parse into the same blocking/spawn_blocking path as the lock work.

• Export file permissions are only private on Unix. write_export sets mode(0o600) under #[cfg(unix)]; on Windows the file will have default permissions. If cross-platform owner-only semantics matter, add a Windows ACL/fixup path or document the Unix-only guarantee.

• Minor: the offline import path parses the interchange twice: interchange_chain and then import. Since StagedHistory is already public, run could call stage once and then absorb(Some(&staged), &[]) after opening the record.

• Minor: absorb commits even when the imported history changes no watermarks. Repeatedly importing the same large history rewrites the durable record; it could compare staged records against current watermarks and skip unchanged entries.

---

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

[pablodeymo]

Addressed the bot round in 869c9fa2, 4963cf7c and 22819e24, on top of the rebase onto #734's current head.

Fixed

• Duplicate pubkeys (Claude, Gemini, Grok, Kimi, DeepSeek): export lists a key once however many times it is requested. Tests for a DELETE naming a key twice and for export itself.
• Unused H256 import (Grok, Kimi): removed. It failed make lint but not CI, because the workflow's by-name clippy step did not list the new target; it does now.
• Double parse in the CLI import (Claude, Gemini, DeepSeek): staged once, interchange_chain is gone.
• Interchange parse on the async worker (Grok, Kimi, DeepSeek): runs on the blocking pool.
• Re-import of a deleted key with no history (Grok): warned, with the record it continues on.
• Missing raise-over-existing test (Grok) and history-for-a-key-outside-the-request test (Claude): added.
• Crash between saving a key and registering it (Codex, P1): the history is now written before any key is saved. A key deleted earlier keeps its record, so keys-first plus a crash would have loaded it on its pre-delete watermarks. History only ever raises watermarks, so writing it for keys that then fail to save leaves nothing dangerous behind. history_is_written_before_its_key_is_saved is the regression test.
• History raising an active key's watermarks (Claude, DeepSeek): kept, since a history saying a running key signed elsewhere is exactly what the record must know, and now logged per key.
• Lock-ordering invariant on DELETE (Claude, Grok): holds, since the duty services record before taking the store's read lock. Stated in the comment.
• absorb rewriting the record when nothing changed (DeepSeek): skipped now.

Not changed, on purpose

• Failed absorb stranding keystores (Grok, DeepSeek): undo_import already rolls them back, pinned by a_failed_registration_rolls_the_import_back.
• Export failure on DELETE after the keys are gone (Claude, Grok, DeepSeek): kept Prysm's answer. The records stay, so a retry answers not_active with the history. The comment now says so.
• Skipping a refused step's blocks in the vector runner (Grok): the README says to continue, and the only refused-step vector has no blocks or attestations.
• sha256sum portability and the archive checksum (Claude, Grok): same pattern the existing cryptography-specs target uses; macOS ships sha256sum.
• Windows export permissions, stringly-typed statuses, fsync of the export's parent directory, the "make test runs first" doc line: nits or pre-existing conventions, left as they are.

[MegaRedHand]

Re-reviewed at 135c9e2. Everything from the previous round is addressed. Writing the history before any key is saved is a better fix than the single write I suggested: a deleted key keeps its old record, so saving the keys first and crashing before the history landed would restart that key on its old record.

Three comments inline, plus one on a test outside this diff:

• an_import_carrying_history_is_refused_whole (keystores.rs, from #734) still passes here, but only because "{}" is not a valid interchange. Its name and doc describe #734's behaviour of refusing any history. Renaming it to something like an_import_whose_history_does_not_parse_is_refused_whole, with the doc to match, would keep it meaningful.

[MegaRedHand]

This skips the rest of the case whenever a step with contains_slashable_data is refused. The README allows that, but it is wider than what this importer refuses. At v5.3.0, 21 steps qualify, and only 4 of them (the single_validator_source_greater_than_target* cases) hold an attestation whose source is at or past its target. If the importer ever started refusing one of the other 17, for example a duplicate-pubkey or surround step, the suite would still pass, with up to 49 block and attestation checks skipped.

Could the skip apply only when the step's interchange holds such an attestation (or when the error is the "at or after its target" refusal), so that every other slashable step must import?

[MegaRedHand]

This parses other clients' files with Interchange, whose doc (lines 45-48) says it is for this client's own file only and that an importer needs its own, lenient, types. The strictness itself seems fine: Lighthouse's EIP-3076 types are deny_unknown_fields too. I'd keep the behaviour and update that doc to say the types are shared by the record reader and the importer.

[MegaRedHand]

Since #734 locks the directory itself (81bdc45), export no longer needs a writable directory: flock works on a directory opened read-only, and an export writes nothing to the directory. I checked the lock on a mode 0500 directory. The "so the directory must be writable" part can go.

[MegaRedHand]

LGTM