[pablodeymo]

🗒️ Description / Motivation

ethlambda validator kept no slashing-protection record. Its two in-memory guards (AttestationGuard, ProposalGuard) applied EIP-3076's minimal rule within one run, but a restart emptied them and nothing stopped a second process from signing with the same keys. The client warned about this on every start, and docs/spec_deviations.md listed it as a known gap. That gap is the blocker for anyone running ethlambda with real stake.

This PR makes slashing protection durable. It is the first of two PRs; the second adds EIP-3076 interchange import/export (CLI and keymanager API).

Design. Before choosing, we surveyed Lighthouse, Prysm, Teku, Nimbus, Lodestar and Grandine.

• Teku's model: one record per validator holding EIP-3076 minimal watermarks (highest block slot, highest attestation source and target). It persists the rule our guards already enforced.
• Why not full history: the full-history clients end up enforcing almost the same thing. Lighthouse prunes to one epoch and minifies every import. What full history buys is allowing a byte-identical re-sign after a crash, and it costs a new C dependency (SQLite), pruning and migrations.

What Changed

• New crates/validator/src/slashing_protection/:
  • mod.rs, SlashingProtection:
    • the rule: an attestation needs target > last target and source >= last source; a block needs slot > last slot;
    • "never signed" is None, so slot 0 and epoch 0 are signable exactly once;
    • open and the batch APIs;
    • every change is written to disk first and taken into memory only once the write succeeds.
  • file.rs, the on-disk record:
    • <validators-dir>/slashing_protection.json is an EIP-3076 v5 minimal interchange naming the chain's genesis_validators_root;
    • strict reading that fails closed;
    • durable writes: temp file, fsync, rename, directory fsync;
    • an exclusive File::try_lock on slashing_protection.lock.
• Signing path:
  • attestation.rs: one slot's batch is checked and recorded in a single write, inside spawn_blocking and before the store's read lock is taken. Only then are the allowed attestations signed.
  • proposal.rs: the cheap pre-check stays before produce_block. The recording check moves before signing and persists.
  • Write failure: a failed write signs nothing.
• Startup (lib.rs):
  • the record is opened right after genesis(), which supplies the root it must match;
  • a key with no record is refused unless --init-slashing-protection is passed;
  • the unconditional "keeps NO slashing-protection record" warning is replaced by an info line.
• Keymanager: POST /eth/v1/keystores registers each imported key before writing it to disk or loading it.
• Removed: attestation_guard.rs and proposal_guard.rs. Their rule and tests moved into slashing_protection.
• New metrics: ethlambda_validator_slashing_protection_write_seconds and ethlambda_validator_slashing_protection_write_failures_total.
• Docs:
  • docs/spec_deviations.md: the section is rewritten to describe the minimal-watermark deviation.
  • docs/cli.md and README.md: the new flag and the slashing-protection callout.
  • docs/metrics.md and CLAUDE.md: updated to match.

Correctness / Behavior Guarantees

• Ordering: check, then a durable record, then the signature. A signature that exists is always one the record already accounts for, whether the process then crashes, hits its duty deadline, or is rejected by the beacon node.

• Fail closed: startup stops on any of these, never reading them as "nothing signed":

  • an empty, malformed or wrong-version file;
  • a validator listed twice;
  • a record for another genesis_validators_root;
  • a key with no record.

  At signing time, an unregistered key or a failed write refuses the message.

• One process: a second validator client on the same --validators-dir refuses to start while the first holds the lock.

• Behavior change for operators: the first start after upgrading refuses with a message naming --init-slashing-protection. That flag registers each key as never having signed. It is meant only for keys that have signed nothing anywhere for at least two epochs, and it never lowers an existing record; the migration window is left to doppelganger protection, in a later PR.

• Behavior change for deployments: --validators-dir must now be writable, for the record and its lock. A read-only mount of the keys (a Kubernetes Secret, a :ro volume) fails at startup.

• Known cost: a byte-identical re-sign after a crash is refused, as in Teku. Documented in spec_deviations.md.

• Not in this PR: carrying history in or out. Keymanager DELETE still returns the empty, all-zero-root interchange, and POST still ignores slashing_protection. Both are PR 2.

Tests Added / Run

• slashing_protection unit tests:
  • every rule case ported from the guards;
  • reopening restores the record;
  • each corrupt-file shape fails open;
  • a validator listed twice fails open;
  • another chain is refused;
  • the lock refuses a second open and is released on drop;
  • an unknown key is refused without init and registered with it;
  • init never lowers a record;
  • a failed write (read-only dir) changes nothing;
  • the file is a valid EIP-3076 v5 minimal interchange;
  • a key twice in one batch is refused the second time.
• Service tests (MockBeaconNode):
  • a_restart_does_not_forget_what_was_attested;
  • a_restart_does_not_forget_what_was_proposed (no block is even requested);
  • a_record_that_cannot_be_written_signs_nothing.
• Keymanager: an_imported_key_is_registered_for_slashing_protection, where the next start opens without --init-slashing-protection.
• Commands run:
  • make fmt: clean
  • make lint: clean
  • make test: 2878 passed, 0 failed (after the review follow-ups); the validator crate alone is at 344 after the bot follow-ups

Review follow-ups (2808b51)

Review point	Change
Kurtosis battery would fail on main	vc_extra_params: ["--init-slashing-protection"] on both ethlambda validator clients. The Assertoor check on this PR fails for the same chicken-and-egg reason: PR runs test the published unstable image, which does not know the flag yet. The post-merge run on main tests the merge commit's image, which does
Keys registered without history start as "never signed"	Kept, by design. Seeding the current slot and epoch was added after the first review and dropped after the second: it only covers messages signed elsewhere up to this node's clock, costs the rest of the epoch's duties on every registration, and would differ from the interchange import's empty entries. The migration window is doppelganger protection's, in a later PR
path.exists() fails open	Only NotFound counts as absent. A symlink, directory or unreadable path is refused instead of replaced
A failed import leaves a record behind	Keys are registered in one write, after they are persisted and before any is loaded
Mutex taken on the executor	Every async access (attestations, the proposal pre-check and record, the keymanager) goes through one slashing_protection::blocking helper
Read-only --validators-dir	Documented in docs/cli.md
Refusal counter undercounts	A failed write counts every refused attestation
Permission tests fail as root	Skipped when geteuid() == 0
Lock described as per key	The docs now say it is per directory
Stale "guard" wording	Replaced, including ProposalRefused and is_retryable
Startup error pointed at an import that does not exist yet	Reworded to the wait condition (import arrives in #736)
Cleanups	The durable write moved to secure_fs::write_private_durably and is shared with validator_definitions.yml, which gains its missing fsyncs. No clone of the record per write, compact JSON, one write per keymanager import

Bot review follow-ups (696e2a8)

Review point	Change
macOS: directory fsync is not F_FULLFSYNC (Kimi 4)	The directory flush after the rename calls File::sync_all first, F_FULLFSYNC on macOS like the file's own flush, and falls back to a plain fsync only where a filesystem refuses that for a directory. The original error is kept if both fail
File mode and the deterministic .tmp path (Kimi 5, 11)	The umask itself was not a problem (it can only clear bits from 0600), but a leftover .tmp from a crashed write kept its old mode, since mode() applies only at creation. The temporary is now removed first and created with create_new, so the record never inherits a wider mode and a symlink planted at the .tmp path is never written through. Two tests pin both
Poison vs. disk failure, string errors (Kimi 1, 9)	slashing_protection::blocking returns the crate's Error instead of a String. A poisoned lock, or a panic inside the operation, is the new SlashingProtectionUnusable, whose message says the client must be restarted; a failed write stays Io. Never retryable
Duplicate-key wording (Kimi 2)	The doc now says a key's later position is judged against its earlier one's verdict
attest answers Ok with zero published on a record failure (Codex)	Attested gains a refused count, so a slot where protection withheld every signature, or could not record them, is distinguishable from a slot with nothing to publish. The refusal tests assert it
Keymanager ignores imported history (Codex)	That is #736
Already done in 2808b51 (Kimi 3, 12)	No clone per write; compact JSON

Related Issues / PRs

• Follow-up: EIP-3076 interchange import/export (CLI and keymanager API), with the eth-clients/slashing-protection-interchange-tests v5.3.0 vectors.
• 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]

🤖 Claude Code Review

Review of PR 734

The only change in the working tree is a 4-line edit to CLAUDE.md. It makes the documentation wrong, so I recommend not merging it as written.

CLAUDE.md:846-850: the edit removes true documentation

The diff replaces the description of durable slashing protection with "still keeps no slashing-protection record". The code on this branch has that protection:

• crates/validator/src/slashing_protection/ exists and has mod.rs and file.rs.
• signing.rs, attestation.rs, proposal/http_api and bin/ethlambda/src/validator.rs all use it.
• docs/cli.md:288 documents --init-slashing-protection.
• mod.rs:277-287 refuses to start when a key has no record.

The text being removed was accurate. It said the client keeps EIP-3076 minimal-watermark protection in --validators-dir/slashing_protection.json and makes it durable before every block and attestation signature. That is the project's main safety guarantee for validators.

The new text says the opposite. Operators or agents who read CLAUDE.md could wrongly conclude the client is unsafe to run, or that a startup refusal is a bug. They could also re-implement an in-memory guard that already exists.

Suggested fix: revert the CLAUDE.md hunk.

Caveats

• git show HEAD lists the whole repository (536 files), so I could only review the uncommitted working-tree diff. If this PR is meant to carry other code changes, they weren't visible to me.
• If the intent was to drop the slashing-protection feature, the code, docs/cli.md and the --init-slashing-protection flag would all need to change with it. That would also be a serious safety regression and would need its own justification.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces durable slashing protection to replace in-memory guards. This is critical consensus-layer code affecting validator safety.

Security & Correctness

1. spawn_blocking error handling is fragile (crates/validator/src/attestation.rs:156-168, crates/validator/src/proposal.rs:274-286)

Both locations follow this pattern:

let verdicts = tokio::task::spawn_blocking(move || { ... })
    .await
    .map_err(|err| err.to_string())
    .and_then(|verdicts| verdicts);

Issue: spawn_blocking panics are converted to String errors and treated as "refuse all." However, JoinError from spawn_blocking can indicate the task panicked, which may mean the lock was poisoned or that check_and_record_attestations panicked after partially modifying state.

More critically: if the spawn_blocking task panics while holding the std::sync::MutexGuard, the mutex becomes poisoned. The next lock acquisition will fail. But the current code doesn't distinguish between:

• Lock poison (permanent, requires restart)
• Disk write failure (potentially transient)

Suggestion: Distinguish these cases. A poisoned lock should probably crash the process or at least be logged at error! with clear operator guidance, since it indicates a bug.

2. check_and_record_attestations allows duplicate pubkeys with subtle semantics (crates/validator/src/slashing_protection/mod.rs:303-320)

pub fn check_and_record_attestations(
    &mut self,
    pubkeys: &[BlsPubkey],
    data: &AttestationData,
) -> Result<Vec<std::result::Result<(), Refusal>>> {

The doc comment says: "A key listed twice is judged against its first verdict, so it is refused the second time."

Issue: This relies on staged.get(pubkey) finding the first inserted record. However, HashMap iteration order is not deterministic. While staged.get will find some record, the "first verdict" semantics are only guaranteed because the loop processes sequentially and staged is checked before self.records. This is correct but subtle.

However: The test a_key_twice_in_one_batch_is_refused_the_second_time verifies this. The implementation is correct but the comment could be clearer that "first" means "earlier in the pubkeys slice."

3. commit clones entire HashMap on every write (crates/validator/src/slashing_protection/mod.rs:352-365)

fn commit(&mut self, changes: HashMap<BlsPubkey, Record>) -> Result<()> {
    let Some(storage) = &self.storage else {
        self.records.extend(changes);
        return Ok(());
    };
    let mut next = self.records.clone();  // ← O(n) clone every time
    next.extend(changes);
    // ... write ...
    self.records = next;
    Ok(())
}

Issue: For large validator sets (e.g., 10,000 keys), cloning the entire HashMap on every attestation batch and every block proposal is O(n) per operation. With one attestation batch per slot and potentially multiple blocks, this could be significant.

Suggestion: Consider using im::HashMap or a similar persistent data structure, or at least benchmark this. For now, document the expected validator count limits.

4. sync_directory uses libc::fsync not F_FULLFSYNC on macOS (crates/validator/src/slashing_protection/file.rs:282-293)

#[cfg(unix)]
fn sync_directory(directory: &Path) -> std::io::Result<()> {
    use std::os::fd::AsRawFd as _;
    let directory = File::open(directory)?;
    // SAFETY: the descriptor is owned by `directory`, which outlives the call.
    if unsafe { libc::fsync(directory.as_raw_fd()) } == 0 {
        Ok(())
    } else {
        Err(std::io::Error::last_os_error())
    }
}

Issue: The comment explains this avoids F_FULLFSYNC because "some filesystems refuse it for a directory." However, on macOS with APFS, fsync does not guarantee durability—only F_FULLFSYNC does. This is a known macOS behavior that has caused data loss in other blockchain clients.

Trade-off: The comment acknowledges this is intentional. But for a slashing protection record, durability is the entire point. Consider:

• Using F_FULLFSYNC by default and falling back to fsync only on the specific error
• Or documenting that macOS is not recommended for production validators

5. open_private sets 0600 but umask may interfere (crates/validator/src/slashing_protection/file.rs:251-264)

#[cfg(unix)]
fn open_private(path: &Path) -> std::io::Result<File> {
    use std::os::unix::fs::OpenOptionsExt as _;
    std::fs::OpenOptions::new()
        .write(true)
        .create(true)
        .truncate(true)
        .mode(0o600)
        .open(path)
}

Issue: .mode() sets the creation mode, but the process umask still applies. If umask is 0077, the file becomes 0600 & ~0077 = 0600. But if umask is 0002 (common), it's still 0600. This is generally correct, but consider explicitly chmoding after creation to be certain, or document that the validators directory should have restricted permissions.

Correctness: Fork Choice & Consensus

6. No issues found in consensus-critical paths

The slashing protection correctly implements EIP-3076 minimal watermarks:

• Block: slot > recorded_slot (strictly greater)
• Attestation: target > recorded_target AND source >= recorded_source

The attestation check also validates source <= target (though this should never happen from an honest beacon node).

The separation of block and attestation watermarks is correct—a validator can propose and attest in the same epoch.

Rust Best Practices

7. std::sync::Mutex in async code with spawn_blocking is correct but worth noting

The pattern used throughout:

let protection = self.protection.clone();
tokio::task::spawn_blocking(move || {
    let mut protection = protection.lock().map_err(...)?;
    // ... do work ...
}).await

This is the correct way to use a synchronous mutex in async Rust. The spawn_blocking prevents executor blocking. Good.

8. Arc::into_inner in tests (crates/validator/src/slashing_protection/mod.rs:468-472)

fn memory() -> SlashingProtection {
    Arc::into_inner(SlashingProtection::in_memory())
        .expect("sole owner")
        .into_inner()
        .expect("not poisoned")
}

Issue: This will panic if any clone of the Arc exists. The test helper is only used in synchronous tests that don't share the Arc, so it's fine. But it's fragile—consider Arc::try_unwrap with a better error message, or document the precondition.

9. Error type conversion loses structured information

Multiple locations convert errors to strings:

.map_err(|err| err.to_string())

This makes it impossible for callers to programmatically distinguish error types. For slashing protection, where operators need clear guidance (e.g., "disk full" vs. "another process has the lock"), consider keeping structured errors longer in the pipeline.

Minor Issues

10. file.rs missing use std::os::unix::fs::PermissionsExt in test cfg

The #[cfg(unix)] tests in mod.rs use PermissionsExt, but this is in the test module of mod.rs, not file.rs. No issue.

11. temporary_path could collide (crates/validator/src/slashing_protection/file.rs:245-249)

fn temporary_path(path: &Path) -> PathBuf {
    let mut name = path.file_name().unwrap_or_default().to_os_string();
    name.push(".tmp");
    path.with_file_name(name)
}

If two processes write concurrently (which shouldn't happen due to the lock, but consider crashes), the temp file name is deterministic. With the lock, this is fine. If the lock were somehow bypassed, there'd be a collision.

12. serde_json::to_vec_pretty for durability writes

Pretty-printed JSON is larger and slower to parse than compact JSON. For a file rewritten every slot, consider to_vec (compact). The pretty format aids debugging but at a cost.

Documentation & Comments

13. Excellent documentation throughout

The module-level docs in slashing_protection/mod.rs, file.rs, and the updated spec_deviations.md are thorough and accurate. The warnings about --init-slashing-protection are appropriately scary.

Summary

Severity	Count	Items
Critical	0	—
High	1	Item 4: macOS fsync vs F_FULLFSYNC durability concern
Medium	2	Item 1: spawn_blocking panic handling; Item 3: O(n) clone per write
Low	4	Items 5, 8, 9, 11, 12

The implementation is fundamentally correct and well-structured. The most significant concern is the macOS durability trade-off (Item 4), which should be documented more prominently or revisited. The O(n) clone (Item 3) should be benchmarked for expected validator counts.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The PR materially improves validator safety by moving attestation/proposal guards to durable pre-sign recording, using atomic replace + fsync semantics, and sharing the guard with key import. I did not spot a consensus-critical bug in fork choice, state transition, SSZ, or signature logic because this PR stays in the validator client path.

• No correctness issue found in the core slashing-protection flow: attestation recording happens before signatures in crates/validator/src/attestation.rs:152, block recording happens before signatures in crates/validator/src/proposal.rs:269, and startup rejects unregistered keys unless explicitly initialized in crates/validator/src/lib.rs:182.
• The file handling is appropriately fail-closed and durable: strict parse/version/root checks in crates/validator/src/slashing_protection/file.rs:102, atomic temp-write/rename/fsync in crates/validator/src/slashing_protection/file.rs:184, and single-process locking via a dedicated lockfile in crates/validator/src/slashing_protection/file.rs:43.
• The main security tradeoff is documented and intentional: POST /eth/v1/keystores still ignores imported EIP-3076 history and registers keys as “never signed” in crates/validator/src/http_api/keystores.rs:249. That is acceptable only because the API warns and the docs are explicit, but it remains an operational footgun if an operator migrates hot keys from another client.
• Minor maintainability concern: AttestationService::attest suppresses all signing when durable recording fails and returns Ok(Attested { published: 0, ... }) rather than an error in crates/validator/src/attestation.rs:171. That matches the safety goal, but it also means the caller cannot distinguish “no duties published” from “storage/pathological protection failure” except via logs/metrics. If that distinction matters operationally, consider surfacing a dedicated error or status.
• Performance looks reasonable: one durable write per attestation batch and per block, offloaded via spawn_blocking in crates/validator/src/attestation.rs:160 and crates/validator/src/proposal.rs:275. That is a good tradeoff for validator safety; I do not see an avoidable hot-path regression beyond the expected fsync cost.
• I could not run cargo test because the sandbox prevented rustup from creating temp files outside the workspace. If you want, I can still do a narrower follow-up pass on the newly added tests only.

---

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

[MegaRedHand]

The Kurtosis assertoor battery will fail on main after this merges.

The pinned lambdaclass/ethereum-package (67cb0e02, src/vc/ethlambda.star) starts ethlambda validator with --validators-dir=/data/ethlambda/validators, which is a fresh directory on every run, and without --init-slashing-protection. tooling/kurtosis-assertoor/network_params.yaml sets no vc_extra_params for the two ethlambda VC participants.

So SlashingProtection::open hits !missing.is_empty() && !init and the VC exits. The block-proposal check for participants 2 and 3 can then never pass. This PR's CI will not show it, because kurtosis_assertoor.yml runs on pull requests only for changes under its own directory or workflow; the failure appears on the first push to main.

Fix: add vc_extra_params: ["--init-slashing-protection"] to both ethlambda VC participants (safe there, since the genesis keys have never signed), or pass the flag in the fork's launcher.

[MegaRedHand]

Keymanager import registers keys as never having signed, which skips the --init-slashing-protection gate and drops the history the caller sent.

register() writes Record::default() for every new key. When the request includes slashing_protection, import only logs a warn!. The CLI calls this exact action DANGEROUS and refuses it without a flag. The API does it on every import, with no flag and no two-epoch condition.

Scenario: a key is deleted from another VC (or from this one after a DELETE) and imported here with its interchange. It is registered with no watermarks and attests in the same epoch, which is a double vote.

Until PR 2, it would be safer to refuse the import when slashing_protection is present, or to seed the new record at the current epoch/slot rather than None.

[pablodeymo]

Done in 2808b51. Keys registered without history (--init-slashing-protection and keymanager imports) now start at the current slot and epoch. One difference from the suggestion: the attestation source stays at 0. The source a beacon node hands out is the justified epoch, which lags the current one, so seeding it at the current epoch would refuse every attestation until justification caught up, and on a non-finalizing chain, forever. The target watermark alone closes the double-vote window. The surround window against older history stays open, which is why the flag's docs still say "two epochs".

[MegaRedHand]

path.exists() fails open. It returns false for a dangling symlink and for any metadata error (for example EACCES). In both cases the record is treated as absent.

Scenario: an operator symlinks slashing_protection.json to a separate volume, and that volume is not mounted at startup. With zero keys at startup (keymanager-only), or with --init-slashing-protection left in the unit file, open writes a fresh empty record. rename then replaces the symlink itself, so the real history is orphaned and every key signs from None.

Suggestion: use symlink_metadata / try_exists. Treat only NotFound as absent, and refuse anything that is not a regular file.

[pablodeymo]

Fixed in 2808b51. Only NotFound counts as absent; a symlink, directory or metadata error is refused instead of replaced (file::exists), with a test for a dangling symlink.

[MegaRedHand]

A failed import leaves a "never signed" record behind, and that record later bypasses the startup gate.

register() commits before persist_import. When persist_import fails (disk, definitions save, symlink refusal), the status is error, but nothing removes the empty record that was just written.

Later the operator adds the same keystore by hand, for example copied from the client that ran it. On restart, open() finds a record, so --init-slashing-protection is never required and the key starts as never having signed.

Suggestion: register only after the key is persisted (keeping the order "record exists before the key is loaded"), or remove the record on failure if this call created it.

[pablodeymo]

Fixed in 2808b51. Keys are registered in one write, after they are persisted and before any is loaded, so a failed import leaves no record (test: a_failed_import_leaves_no_slashing_protection_record).

[MegaRedHand]

The std::sync::Mutex is taken on the executor here (and in keystores::prepare_import), while another holder may be inside an fsync.

The SharedProtection doc says async callers take it inside spawn_blocking. This pre-check does not, and keymanager register does a full rewrite plus two fsyncs inline in an axum handler.

Concrete case: an attestation duty abandoned at its deadline (timeout in attest_and_aggregate) does not cancel the detached blocking task, so its write can still be running at the next slot boundary. propose then blocks the duty-loop worker on lock() for as long as the disk takes.

Suggestion: move the pre-check and register into spawn_blocking too.

[pablodeymo]

Fixed in 2808b51. One slashing_protection::blocking helper is now used everywhere: attestations, the proposal pre-check and record, and keymanager registration (and export in #736).

[MegaRedHand]

This startup error points operators to a path that does not exist. "Import their history from the client that last ran them" is not supported in this PR: there is no CLI import, and the keymanager ignores slashing_protection.

An operator migrating keys therefore has one real option, --init-slashing-protection. That is the dangerous one for a key that signed recently elsewhere.

Suggestion: until PR 2, say that import is not available yet, and spell out the wait condition (stop the old client and wait at least two epochs) as a requirement.

[pablodeymo]

Reworded in 2808b51 to the wait condition (stop the old client and wait two epochs). The import path arrives in #736.

[MegaRedHand]

Stale "guard" wording remains after the guards were removed. Locations:

• here, and proposal.rs:226
• the test docs at proposal.rs:653, 800, 1072
• error.rs:75: the ProposalRefused rustdoc still says "The proposal guard refused"
• error.rs:140: "A refusal is deterministic". That is no longer true, since ProposalRefused now also carries record-write failures.
• beacon_node/fallback.rs:609, 625 and beacon_node/mod.rs:163

[pablodeymo]

Fixed in 2808b51, including ProposalRefused's rustdoc and the is_retryable comment, which now covers record-write failures too.

[MegaRedHand]

Reuse (cleanup). open_private re-implements secure_fs::write_private. secure_fs.rs:8-10 says every file that "decides what a validator signs" must go through that module. write_durably is also a second tmp+rename next to ValidatorDefinitions::save (keys/definitions.rs:54-75), which has no fsync.

Suggestion: add one secure_fs::write_private_durably (tmp at 0600, sync_all, rename, directory fsync) and use it for both files.

[pablodeymo]

Done in 2808b51: secure_fs::write_private_durably is shared by the record and validator_definitions.yml, which gains its missing fsyncs.

[MegaRedHand]

Simplification (cleanup). This block (spawn_blocking, then lock, then map poison to a string, then flatten the JoinError) appears again almost verbatim in proposal.rs:274-285.

Suggestion: one helper in slashing_protection, for example async fn blocking<T>(p: &SharedProtection, f: impl FnOnce(&mut SlashingProtection) -> Result<T> + Send) -> Result<T, String>. It removes the duplicate, and the keymanager and proposal pre-check could use it too (see the executor-blocking comment).

[pablodeymo]

Done in 2808b51: one slashing_protection::blocking helper replaces the duplicated blocks.

[MegaRedHand]

Efficiency (cleanup). Each commit clones the whole records map and rewrites and double-fsyncs the full pretty-printed file, while holding the mutex. So the cost of every slot's batch and every block grows with the total number of keys, not with the batch.

The keymanager makes it worse: register(&[pubkey]) runs once per key, so an import of N keystores does N full rewrites and 2N fsyncs, and attestation writes wait behind them.

Cheaper options:

• Serialize from self.records with changes overlaid, without cloning.
• Use compact JSON.
• Register all decrypted keys of one import in a single commit.

[pablodeymo]

Done in 2808b51: no clone per write (unchanged entries are chained with the changes), compact JSON, and one write per keymanager import.

[MegaRedHand]

Altitude. init (and keymanager registration) seed None, so "safe only if nothing was signed for two epochs" is documentation, not enforcement. open runs after genesis()/spec(), so the current slot and epoch are known.

Seeding block_slot = current_slot and attestation = (current_epoch, current_epoch) would cost at most one epoch of duties. It would close the double-vote window for a key that signed recently elsewhere, both here and on the keymanager path. Worth considering, even if it lands in PR 2.

[pablodeymo]

Done in 2808b51. Keys registered without history (--init-slashing-protection and keymanager imports) now start at the current slot and epoch. One difference from the suggestion: the attestation source stays at 0. The source a beacon node hands out is the justified epoch, which lags the current one, so seeding it at the current epoch would refuse every attestation until justification caught up, and on a non-finalizing chain, forever. The target watermark alone closes the double-vote window. The surround window against older history stays open, which is why the flag's docs still say "two epochs".

[pablodeymo]

Kurtosis battery (review summary): Added vc_extra_params: ["--init-slashing-protection"] to both ethlambda validator clients in 2808b51. The Assertoor check on this PR still fails, but only because PR runs test the published unstable image, which doesn't know the flag yet (unexpected argument '--init-slashing-protection'). The post-merge run on main tests the merge commit's image, which does.

[MegaRedHand]

Drop the seeding and leave the migration window to doppelganger protection (to be added in another PR).

Record::seeded came out of my 10-29 review, but I'd remove it and go back to registering keys with no history as never having signed:

• It only guards against messages signed elsewhere up to this node's now, and it costs the rest of the epoch's duties every time a key is registered.
• It's inconsistent between entry points: feat(validator): EIP-3076 slashing-protection import and export #736's interchange import still registers an empty or partial entry as never signed, so the same key starts differently depending on how it arrived.
• Lighthouse doesn't seed in any of these cases (--init-slashing-protection, keymanager POST without history, empty interchange entries). It registers with no history and covers the migration window with its opt-in doppelganger protection, which checks the network for the key's activity before letting it sign.

What changes: Record::seeded and its callers (open's init path, register), the keymanager warnings in keystores.rs, the flag's help in bin/ethlambda/src/validator.rs, and the "as if it had signed a block and an attestation at the current slot and epoch" wording in docs/cli.md.

[pablodeymo]

Done in 4db7bb0. Record::seeded is gone and open and register no longer take a slot: a key with no history is registered as Record::default() again, at startup and from the keymanager, so slot 0 and epoch 0 are signable once. The keymanager's two import warnings, the flag's help in bin/ethlambda/src/validator.rs and the docs/cli.md row now say "as never having signed", and the module doc says the migration window is doppelganger protection's to close. The seeding test is replaced by one that pins the never-signed start for both entry points. PR description updated to match.

[MegaRedHand]

This fallback also runs on Linux, where it can turn a failed flush into Ok.

On Linux, File::sync_all is already fsync, so after a failure this retries the same call on the same descriptor. Retrying fsync after an error is not reliable there: the kernel reports a writeback error once per open file, so the second call can return 0 even though nothing reached the disk (the PostgreSQL "fsyncgate" behavior). Whether that happens for a directory depends on the filesystem; I haven't reproduced it.

If it does, write_private_durably returns Ok, commit treats the record as durable, and the block or attestation is signed. After a power loss the rename can be lost, the record goes back to the older watermark, and the restarted client can sign the same slot or target again.

The fallback only exists for macOS (F_FULLFSYNC refused for a directory), so gating it with #[cfg(target_os = "macos")] fixes this without changing behavior there.

[pablodeymo]

Agreed, and done in 4db7bb0. The plain fsync retry is now under #[cfg(target_os = "macos")], so on Linux sync_directory is a single sync_all whose error is returned as is. The doc comment spells out why a second fsync after a failed one must not be trusted there. Behaviour on macOS is unchanged.

[MegaRedHand]

Reviewed at 4db7bb0. The signing path looks solid: every block and attestation is checked and durably recorded before it is signed, and a failed write signs nothing. Comments inline.

Follow-up for a separate PR (not blocking this one): validate_attestation_data checks the slot and target epoch but passes data.index through unchecked. Teku (validateCommitteeIndexValue) and Lodestar (validateAttestationDuty: 0 at electra, 0 or 1 at gloas) refuse an index that cannot be valid for the fork. It is a liveness check rather than a slashing one, since protection already refuses a second vote for the same target, and it is a few lines next to the existing checks.

[MegaRedHand]

If register fails here, persist_import has already written the key and added it to validator_definitions.yml as enabled. The response says "saved, but not loaded", but the next restart hits the no-record check in SlashingProtection::open and refuses to start the whole validator client, not just this key.

Failing closed is right for a crash between persist and register, but on this path we know registration failed and can still clean up. Could we undo persist_import for the persisted keys when registration is Err (remove the definitions entries and the keystore and password files), so a failed write costs only this request?

[pablodeymo]

Done in 0ef17bf. When the registration write fails, the keys this request added are taken off disk again: their definitions entries in one save first, since that file is what the next start loads from, then the keystore and password files. A key the file already listed is left as it was (its entry and keystore were rewritten, not added), and its status keeps the "saved, but not loaded" wording. a_failed_registration_rolls_the_import_back forces the write to fail with a directory in the record's place, so it runs the same under root.

[MegaRedHand]

Pre-existing on main, but this comment made me look: the guard is dropped at line 247, before context.store.write(). On the multi-threaded runtime, a DELETE of the same key can take the definitions lock in that gap and queue for the store lock first. It then finds the key on disk but not yet in memory, removes it, and answers deleted. finish_import then inserts it, and it signs from memory until the next restart, although the operator was told it is gone.

The window is tiny, but the fix is free: release the guard after store.write() (or after finish_import). delete already takes the two locks in that order, so there is no deadlock risk.

[pablodeymo]

Done in 0ef17bf: the definitions guard is now held across the store lock and released after it, in the order delete takes the two. The comment above the lock explains the race it closes.

[MegaRedHand]

This still describes the seeding that 4db7bb0 removed. Keys registered by --init-slashing-protection or a keymanager import now start as never having signed (Record::default()), so this does not close the double-vote window for a key that signed elsewhere a moment ago. Line 97 makes the same claim for imports.

It should say the window is left to doppelganger protection, which docs/cli.md lists as not implemented yet, and adjust the guidance on when the flag is safe accordingly. (#736 is still based on the pre-4db7bb03 version and uses Record::seeded; whichever way that lands, these docs should match it.)

[pablodeymo]

Fixed in 0ef17bf, both places. Thanks for catching it: I had only reworded the copy of that paragraph on #736's branch.

[MegaRedHand]

Nothing reads clock since 4db7bb0 dropped the seeding, and the doc still says it dates the record of an imported key. Either remove the field and its initializers, or keep it if seeding comes back with #736. Either way, the doc should match what it does.

[pablodeymo]

Removed in 0ef17bf, with its initializer in lib.rs and the two test ones. Nothing dates a registration any more.

[MegaRedHand]

Nit: this is pub, treats every unknown key as registered and persists nothing, so it is a footgun for any non-test caller. ethlambda-rpc's tests use it, so cfg(test) will not do, but a test-utils feature would.

Related: since most service tests use this, the Refusal::Unregistered path in attest/propose has no service-level test, only the record-level an_unregistered_key_is_refused_on_a_persisted_record.

[pablodeymo]

Done in 09d1cef: in_memory is behind a test-utils feature, which the rpc crate enables as a dev-dependency. The Refusal::Unregistered path now has a service-level test too, an_unregistered_key_is_refused_before_a_block_is_requested in proposal.rs, over a persisted record opened with no keys.

[MegaRedHand]

No test feeds record_of more than one entry of a kind, yet reducing them to their maxima is what keeps a file with several entries for one validator from lowering a watermark. A small test would pin it: attestations [(5, 9), (2, 4)] and blocks [7, 3] should read back as (5, 9) and 7. The deleted guards had recording_out_of_order_does_not_lower_the_bar tests, and this is where that property lives now.

[pablodeymo]

Added in 0ef17bf, with your values: blocks [7, 3] and attestations [(5, 9), (2, 4)] read back as 7 and (5, 9).

[MegaRedHand]

Nit: this maps the blocking result to a refusal, increments inc_blocks_refused and returns ProposalRefused, exactly like the block at line 173. The two copies already differ: the first logs a refusal at warn and this one does not. A small helper returning Result<()> would keep them together.

[pablodeymo]

Done in 0ef17bf. I took the middle option: off unix sync_directory returns Unsupported, so the durable write fails rather than answers Ok without a flush, and the doc on write_private_durably states the guarantee as unix-only. A compile_error! would have made the whole crate unix-only, which seemed more than the function warrants.

[MegaRedHand]

Nit: on non-unix this reports success without flushing, while write_private_durably's doc promises that Ok means the data is on disk. Nothing builds for Windows today, so the simplest fix is to state the guarantee as unix-only, or compile_error! on non-unix, rather than return Ok.

[pablodeymo]

Done in 0ef17bf: one refuse_unless_safe helper, which also makes the two paths log the same way (a judged refusal at warn, a record that could not be consulted or written at error).

[MegaRedHand]

Not for this PR, just a possible follow-up. Once the mutex is poisoned, every duty is refused until restart, but the process keeps running, so a supervisor never restarts it. Exiting on poison would let systemd or k8s apply the remedy this message asks for.

Also minor: the call whose own panic poisoned the lock gets "an earlier access panicked", which points at the wrong event.

[pablodeymo]

Agreed on both, and left for a follow-up PR as you suggest: exiting on poison so a supervisor restarts the client, and attributing the first failure to its own panic rather than to "an earlier access".

[pablodeymo]

Thanks for the third pass. Everything inline is in 0ef17bf and 09d1cef. The two follow-ups you flagged for separate PRs, validate_attestation_data checking data.index the way Teku and Lodestar do, and exiting on a poisoned record, are noted and not in this one.

[pablodeymo]

/review

[github-actions]

🤖 Codex Code Review

1. [P1] Two idle epochs do not make initialization safe. validator.rs:85 recommends initializing previously used keys after two epochs, but this only prevents overlapping target epochs. A previous vote (source=10, target=12) and a later vote (source=9, target=15) still form a slashable surround vote. A stale beacon node can supply that later source. Require prior history or enforce a conservative source watermark during migration; update the CLI warning and documentation accordingly.

2. [P1] A failed import can leave a key enabled on disk. keystores.rs:476 assumes a failed definitions save leaves disk unchanged, but the new durable writer can fail flushing the directory after renaming the updated file. Rollback only changes memory. For a previously deleted key whose protection record remains, import reports an error, yet restarting loads the enabled key. An operator retrying elsewhere can then run two signers. Handle post-rename failures explicitly and durably restore the previous definitions before reporting an import as rolled back.

Tests could not run: Rustup attempted to write to its read-only toolchain directory.

---

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

[github-actions]

🤖 Claude Code Review

Review: durable EIP-3076 minimal slashing protection

This is a solid change. Several things are done right:

• Write-before-sign ordering. Check, durable write, then sign.
• Commit-on-success. commit writes the file first and updates memory only after the write succeeds.
• Fail-closed reads. An empty, truncated, wrong-version, duplicated, symlinked or other-chain record stops the client.
• One write per slot batch.
• Lock on a separate file. This is correct given the rename-based writes.
• Import rollback. It is carefully thought through.
• Test coverage of restart, permission-failure and rollback shapes is good.

The items below are what I would address or consider.

Correctness / security

1. A bad beacon-node answer now poisons the watermark permanently (slashing_protection/mod.rs, Record::check_attestation; beacon_node/mod.rs, validate_attestation_data).

   • validate_attestation_data pins slot and target.epoch. The only check on source.epoch is source <= target.
   • So a buggy or hostile node can answer source == target == current_epoch. That attestation is allowed and recorded.
   • Every honest attestation afterwards has source < current_epoch and is refused as SourceRegressed until justification catches up. That could be many epochs, or never during non-finality.
   • With the old in-memory guard a restart cleared this. Now it survives restarts, and the only recovery is hand-editing the JSON.
   • Suggested fixes:
     • Reject source.epoch >= target.epoch unless target.epoch == 0 in validate_attestation_data, which is never valid outside genesis.
     • Optionally, treat a source far ahead of the previously recorded one with suspicion.
   • Also document a recovery procedure for a wedged watermark in spec_deviations.md.

2. Non-unix regression (secure_fs.rs, sync_directory for not(unix); keys/definitions.rs, save).

   • write_private_durably always fails off unix, and ValidatorDefinitions::save now goes through it.
   • So on Windows every keymanager import/delete fails.
   • SlashingProtection::open also fails at startup whenever it needs to write, so the validator cannot start there at all.
   • If non-unix is unsupported, make that explicit: a startup error with a clear message, or a docs note. Otherwise give definitions.save a non-durable fallback, since that file previously worked there.

3. Misleading import status for re-imported, already-loaded keys (http_api/keystores.rs, the (Ok((pubkey, _)), Err(err)) arm).

   • A key in known_before already has a record (startup required it), so register never fails because of it.
   • In a mixed batch where a new key's registration write fails, the known key is reported as "saved, but not loaded". It is still loaded in the store from startup.
   • Suggest one of:
     • Route known_before keys through finish_import, which should yield the duplicate status.
     • Word the message so it does not claim the key is unloaded.

4. --init-slashing-protection left on permanently (slashing_protection/mod.rs, open).

   • The help text says "remove it once the keys are registered". Operators who bake it into a systemd unit (as the kurtosis config does) will silently register any keystore hand-copied in later.
   • Consider a warn! when the flag is set but missing is empty: "--init-slashing-protection is set but registered nothing; remove it".
   • That makes the stale flag visible.

Performance

5. Full rewrite plus two fsyncs per slot and per block (slashing_protection/mod.rs, commit; file.rs, write).
   • This is O(N validators) JSON encoding and I/O every slot. It is fine for tens to hundreds of keys.
   • For thousands of keys on network-attached volumes (EBS, Kubernetes PVs), fsync latency in the tens to hundreds of ms lands on the proposal critical path, between production and signing.
   • Not blocking, but:
     • The SLASHING_PROTECTION_WRITE_SECONDS buckets top out at 0.5s; add 1, 2.5 and 5 so slow disks are visible rather than all in +Inf.
     • Note the scaling limit in the module docs.

Rust / build

6. File::try_lock / TryLockError (file.rs, lock) were stabilized in Rust 1.89. Make sure rust-version/the toolchain file reflects that, or use fs4/libc::flock.

7. Cargo.toml layout (crates/validator/Cargo.toml). The new [features] table is inserted directly above the # O_NOFOLLOW … comment, which documents the libc dependency that follows. Move [features] to the end of the file so that comment stays attached to its dependency table.

8. Leftover temporary file on a failed write (secure_fs.rs, write_private_durably). A failed rename leaves slashing_protection.json.tmp behind. It is harmless, since the next write removes it, but a best-effort remove_file on the error path would keep the directory clean.

Follow-ups (not blocking)

9. The keymanager could now carry history (http_api/keystores.rs).

   • DELETE still returns an all-zero-root empty interchange, although a real minimal record now exists.
   • Exporting the deleted keys' entries with the real root is straightforward with file::write.
   • Accepting slashing_protection on POST is also nearly free. record_of already reduces arbitrary entries to maxima, and merging with max_with never lowers a watermark.
   • Either would close the most likely migration path for the "key with no history" gap.

10. Missing validator index in the log (proposal.rs, refuse_unless_safe). The Err(err) branch does not log validator; include it for parity with the refusal branch.

Overall the change looks good. I would address Item 1 before merge, since durability turns a transient beacon-node fault into a persistent outage. Item 2 also needs a decision before merge.

---

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

[github-actions]

🤖 Gemini Code Review

Solid implementation of durable EIP-3076 minimal-watermark slashing protection. The ordering guarantees (durable file + directory sync before any block or attestation signature is produced, and outside the ValidatorStore lock), fail-closed file validation, lockfile handling, and keymanager rollback logic are all well-designed and thoroughly tested.

One minor edge case worth noting:

1. crates/validator/src/secure_fs.rs:114 — Path::parent() on a single-component relative path returns Some(""), not None

   sync_directory(path.parent().unwrap_or(Path::new(".")))

   In std::path::Path, Path::new("foo").parent() returns Some(Path::new("")) rather than None (None is only returned for / or ""). If validators_dir is ever empty/relative such that path has a single component, path.parent().unwrap_or(Path::new(".")) evaluates to Path::new(""), and File::open("") in sync_directory fails with ENOENT.

   Filtering out an empty parent avoids this footgun:

   let parent = path
       .parent()
       .filter(|parent| !parent.as_os_str().is_empty())
       .unwrap_or(Path::new("."));
   sync_directory(parent)

---

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

[github-actions]

🤖 DeepSeek Code Review

Overall this is a well-structured change: checking, durably recording, then signing is the right order, and the fail-closed record reader/locking are good.

Issues found:

• crates/validator/src/http_api/keystores.rs, around the new if let Ok(registered) = registration block: this looks like it moves registration, but the code later borrows it via match &registration. Result<usize, Error> is not Copy, so this should be if let Ok(registered) = &registration or a match &registration.

• crates/validator/src/slashing_protection/mod.rs, in SlashingProtection::commit: the comment says a failed write leaves both disk and memory unchanged, but secure_fs::write_private_durably can fail after the rename already happened. If the directory fsync fails, the new record is on disk while self.records remains stale. Since the failed message is not signed, this does not by itself allow a double signature, but the invariant is false and the process continues with memory that can diverge from disk. Consider making post-rename fsync failure fatal, reloading the record from disk, or at minimum correcting the comment and making the state explicit.

• Lifecycle gap around deletion: import now registers keys, but the PR does not appear to remove records when keys are deleted through the keymanager API. That keeps deleted keys’ history forever. It is conservative, but it is surprising and will grow the record file over time. If intentional, add a clear code comment and document it; if not, DELETE /eth/v1/keystores should also remove or export the local slashing-protection entries.

---

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

[github-actions]

🤖 Grok Code Review

The record-before-sign design is sound: watermarks fail closed, a failed write does not advance memory, and the restart and rollback tests cover the important cases. A few safety gaps remain.

1. Keymanager import bypasses the init gate and drops history. Startup refuses a key with no record unless --init-slashing-protection is set, but POST /eth/v1/keystores always registers new keys as never-signed and ignores a supplied interchange (crates/validator/src/http_api/keystores.rs, the block around the new registration call). That is the standard migration path (keystore plus EIP-3076 payload). A caller that just signed elsewhere will double-sign, and the only signal is a warning. Until import exists, reject the request when slashing_protection is present, and do not register a brand-new key as never-signed without an explicit acknowledgement equivalent to the CLI flag. DELETE still returns an empty zero-root interchange even though slashing_protection.json is already a minimal v5 file; exporting that validator's real watermark on delete is the safe direction and is now cheap.

2. The process lock is bypassed by replacing the lock file. file::lock (crates/validator/src/slashing_protection/file.rs, lock) opens slashing_protection.lock and try_locks that inode. flock does not survive unlink: delete the lock file (operator cleanup, crash script, or another user who can write the directory) and a second process creates a new inode, locks it, and signs the same keys. Hold the exclusive lock on the validators directory fd (or another inode that is not replaced on each start), and treat a non-regular lock path the same way exists treats the record.

3. Directory fsync failure leaves disk ahead of memory. In write_private_durably (crates/validator/src/secure_fs.rs), rename happens before sync_directory. If the rename is visible and the directory flush fails, commit returns Err and does not extend memory. This attempt does not sign, so it is not itself a double-sign, but the in-process watermarks are now weaker than the file. Poison the record (or apply the changes to memory and refuse further signing until restart) when durability fails after the rename. Do not continue on the old watermarks.

4. A slow flush burns the epoch, and the deadline path hides why. blocking keeps running after the duty future is dropped, so a slot that overruns during fsync still commits the watermark and then never publishes. The next attempt in that epoch is refused. That is the right slash-avoidance tradeoff, but on NFS or a slow volume it becomes a permanent miss, not a retry. attest returns Ok with refused set; confirm the duty loop logs refused and slashing_protection_write_seconds rather than only a deadline counter, and document that --validators-dir must be a local disk.

The minimal rule itself (strictly increasing target, non-decreasing source, strictly increasing block slot, Option so slot/epoch 0 is signable once, independent block vs attestation watermarks) matches EIP-3076 and is covered by the tests. Gloas envelopes and PTC votes staying unguarded is fine if they remain non-slashable.

---

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

[github-actions]

🤖 Kimi Code Review

1. Blocker — [features] swallows the trailing dependency declaration
   crates/validator/Cargo.toml:65-72 inserts [features] before the existing O_NOFOLLOW dependency comment and its following libc.workspace = true declaration. TOML remains in the [features] table, so that declaration is parsed as a feature rather than a dependency, likely causing manifest parsing to fail. Move [features] after all dependency declarations, or reopen [dependencies] before the libc entry.

2. Non-Unix writes fail only after replacing the destination
   crates/validator/src/secure_fs.rs, write_private_durably / sync_directory: on non-Unix, sync_directory always returns Unsupported, but this happens after rename. Consequently, callers receive Err even though the destination was replaced. This makes ValidatorDefinitions::save report failure after committing changes, violates SlashingProtection::commit’s “failed write leaves both exactly as they were” assumption, and prevents persisted slashing protection from starting successfully on those platforms. If non-Unix durability is unsupported, return Unsupported before creating or renaming anything; otherwise implement the platform-appropriate flush.

3. Scalability risk — every signing update rewrites and sorts the entire registry
   crates/validator/src/slashing_protection/mod.rs, SlashingProtection::commit, together with file::write: each slot’s attestation batch serializes all validators, sorts them, rewrites the complete JSON file, and performs file and directory flushes. This is O(total validators) per update rather than O(changed validators), and the latency remains directly on the attestation/proposal path despite spawn_blocking. Large validator installations on slow disks could regularly miss duty deadlines. Consider an append-only durable journal with periodic compaction, exporting EIP-3076 separately, or at least benchmark and document supported validator-count limits.

The central safety design otherwise looks sound: records are committed before slashable signatures exist, failed writes do not update in-memory watermarks on Unix, duplicate keys in one attestation batch are handled, and startup/import paths fail closed.

---

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

[pablodeymo]

Answers to the six automated reviews of 09d1cef. Fixed in ae16928 and 81bdc45 unless noted.

Fixed

• A bad beacon-node answer wedges the watermark (Claude). validate_attestation_data now refuses a source epoch at or past its target outside genesis, with tests, and spec_deviations.md documents the hand-edit recovery for a record that still ends up ahead of the chain.
• Directory flush failing after the rename (Codex, DeepSeek, Grok, Kimi). write_private_durably now reports whether it failed before or after the rename. For the record, an after-rename failure still signs nothing but takes the new watermarks into memory (Error::Unflushed), so what the process holds is never weaker than what is on disk. For the definitions file it counts as written, with a warning: the entry is visible to every reader, and an entry lost to a power loss only removes a key, never a watermark. That also removes the memory-versus-disk gap Codex described in the import rollback.
• Non-unix fails after replacing the file (Kimi, Claude). The durable write is now refused up front off unix, before anything is touched, and docs/cli.md says the validator client is unix only.
• Lock file can be unlinked (Grok). The lock is now flock on the validators directory's own descriptor, the one inode nothing replaces or deletes; slashing_protection.lock is gone, and a test opens the same directory twice.
• --init-slashing-protection left on (Claude): startup warns when the flag is set and every key already has a record.
• Misleading "saved, but not loaded" for a re-imported key (Claude): reworded to say the keystore was rewritten and the key stays as it was.
• Smaller (Claude, Gemini): validator in the error branch of refuse_unless_safe; write-time histogram buckets at 1, 2.5 and 5 s and a cost paragraph in the module doc; the leftover .tmp removed on a failed write; the empty parent of a single-component relative path flushed as "." (Gemini's Path::parent point was right); [features] moved to the end of the manifest; the docs ask for a local disk.
• Two idle epochs (Codex): the flag's help and the docs now say the wait rules out a double vote, not a surround of an older vote, and point at importing the key's history (feat(validator): EIP-3076 slashing-protection import and export #736) instead.

Not changed, with the reason

• Kimi's blocker: [features] does not swallow the libc dependency, since [target.'cfg(unix)'.dependencies] reopens a table before it; the crate builds and CI is green. Moved anyway so the comment stays with its table.
• DeepSeek's "moves registration": binding a Copy field by value does not move the Result; it compiles as written.
• Claude's File::try_lock note: the workspace pins Rust 1.97.1, above the 1.89 stabilization.
• Keymanager history on POST and DELETE, and deleted keys' records being kept (Claude, Grok, DeepSeek): that is feat(validator): EIP-3076 slashing-protection import and export #736, stacked on this PR, where DELETE exports the real record and keeps it so a re-import stays covered.
• The O(keys) rewrite per slot (Claude, Kimi): reviewed and accepted as is for this PR; the cost paragraph and the wider histogram make it visible.
• Grok's slow-flush point: the duty loop already reports the refused count per slot; only the local-disk line in the docs is new.

[pablodeymo]

/review

[github-actions]

🤖 Codex Code Review

No actionable findings. The watermark checks, durable writes before signing, process locking, and fail-closed error handling look sound.

Validation limitation: I couldn’t run the validator tests because Rust toolchain setup attempted to write to the read-only /home/runner/.rustup/tmp directory.

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 734, durable EIP-3076 minimal slashing protection

This is a solid, carefully reasoned change. The ordering is check, then durable record, then sign. Reads fail closed, the lock is per directory, the chain is pinned by genesis_validators_root, and keymanager registration rolls back on failure. The tests cover restart, lock contention, damaged files and failed writes. The points below are mostly edge cases and things to verify, not blockers.

Correctness / security

1. A beacon node can push the source watermark up (liveness, not safety). In beacon_node/mod.rs, validate_attestation_data now bounds source < target == epoch(slot). That stops the source being set past the target. A malicious or buggy node can still answer with source = target - 1 while the real justified checkpoint is older.

   • The record keeps that source.
   • Every honest attestation after it is then refused as SourceRegressed until justification catches up, which may never happen during non-finality.
   • This is a liveness DoS from one bad node, and fallback makes it more reachable.
   • spec_deviations.md mentions the hand-edit remedy. Consider also adding a log/metric that separates SourceRegressed from TargetNotAdvanced, so operators notice this case quickly.
   • Optionally, cross-check source against the finalized/justified checkpoint from a second node when fallback nodes are configured.

2. Keys not in the store are reported as slashing refusals. In attestation.rs, around lines 146–183, candidates are built from every duty with a parseable pubkey, not only keys loaded in the store.

   • With a persisted record, an unknown key becomes Refusal::Unregistered. That path warns "slashing protection judged this attestation unsafe" and bumps attestations_refused_total, a metric documented as "should normally read zero".
   • A key whose record exists but was deleted via the keymanager mid-slot gets its watermark burned, then fails UnknownValidator at signing. This is harmless, but noisy.
   • Suggested fix: take a brief read lock (or a pubkey snapshot) to filter candidates to loaded keys before recording. The rule is still respected, since recording a superset is safe, and the metric keeps its meaning.

3. known_before in keystores.rs (around line 200) includes disabled definitions.

   • Disabled entries were never passed to SlashingProtection::open, because only loaded keys are. They may therefore have no record.
   • If such a key is re-imported and registration fails, it lands in the "keystore rewritten … the key stays as it was" branch and is not rolled back.
   • If persist_import flipped it to enabled, the next start refuses the whole client.
   • This fails closed, but the blast radius is larger than this request. Consider building known_before from enabled entries only, or from protection.records.

4. Rollback can remove more than it needs to. If a batch mixes a key that already has a record (deleted, then re-imported) with new keys, a failed write rolls back all of them. That is safe, but the already-registered key never needed the write. This is minor and acceptable, but worth a comment.

Build / portability

5. Check that libc is a dependency. libc::geteuid (tests) and libc::fsync (macOS sync_directory) are used in crates/validator, but the Cargo.toml hunk does not add libc. Please confirm it is already a direct dependency, with a dev-dependency for the tests. Otherwise this only compiles through a transitive re-export, which will not work.

6. MSRV. File::try_lock and TryLockError in slashing_protection/file.rs::lock were stabilized in Rust 1.89. Make sure rust-version and the CI toolchain match.

7. flock on network filesystems. On NFS/CIFS, flock may be a no-op or local-only, which silently removes the second-process guarantee. The cli.md note already recommends a local disk. Consider stating this lock caveat there explicitly too.

Performance

8. Every write rewrites the whole record. commit re-serializes the whole map and does two fsyncs while holding a std::sync::Mutex on the blocking pool. That happens once per slot and once per block, which is fine for hundreds of keys. The new histogram is the right instrument.

   • The early check_block in proposal.rs is a pure read, but it queues behind an in-flight attestation write.
   • That costs up to one fsync of latency on the proposal path at slot start, which is acceptable, but worth knowing about.

9. Startup warnings scale with key count. open logs one warn! per missing key under --init-slashing-protection. With thousands of keys this floods the log. Consider a single summary line plus debug! per key.

Smaller notes

• SourceAfterTarget is now unreachable. In Record::check_attestation, validate_attestation_data rejects this case first. Keeping it as defense in depth is good; a one-line comment saying so would help.
• Error::Unflushed is handled two ways. ValidatorDefinitions::save treats it as success, while the protection record treats it as failure and refuses to sign. The asymmetry is justified in the comments and is correct: losing a definitions entry only removes a key.
• DELETE could export real history now. DELETE /eth/v1/keystores still returns an empty interchange with an all-zero root. The record is already a valid minimal v5 interchange, so exporting the deleted keys' entries is now nearly free and would make migration out safe. This is a good follow-up.
• Kurtosis flag on restart. Setting --init-slashing-protection permanently in network_params.yaml makes every container restart emit the "set for nothing" warning. This is fine for a devnet, but note it if those logs are asserted on.

Overall the approach is sound. Item 5 should be verified before merge, and Items 1–3 are worth addressing or explicitly documenting.

---

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

[pablodeymo]

/review

[github-actions]

🤖 Codex Code Review

1. [P1] Delete retries can hide a non-durable removal — keystores.rs:668. After the rename succeeds but directory flush fails, deletion returns an error. A retry sees neither a definitions entry nor a loaded key and returns not_found at line 643 without flushing again. A subsequent power loss can restore the entry, reactivating a key already migrated elsewhere and risking slashing. Preserve the pending durability failure across retries, or durably rewrite the definitions before returning not_found. Add a failure-injection test covering this sequence.

2. [P1] Two idle epochs do not make initialization safe — README.md:347. This guidance permits previously active keys to start with empty watermarks after two epochs. During non-finality, an old vote (source=10, target=20) can be surrounded by a new vote (source=9, target=23), despite that wait and no concurrent client. Restrict this recommendation to never-used keys or require preserved history; update the startup error’s equivalent guidance too. The caveat in CLI help should also appear here.

Tests could not run: rustup attempted to write to a read-only toolchain directory.

---

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

[github-actions]

🤖 Claude Code Review

Review: durable EIP-3076 minimal slashing protection

This is a solid change. The ordering is right: the record is checked, then durably written, then the message is signed. Reads fail closed, the lock is on the directory inode rather than a deletable lock file, the chain root is checked, validate_attestation_data gets a matching guard, and the tests cover the restart and failed-write shapes. The points below are mostly edge cases and polish.

Correctness / safety

1. The read and write paths disagree on source == target (slashing_protection/file.rs, record_of; mod.rs, Record::check_attestation).

   • record_of only rejects source > target.
   • check_attestation and validate_attestation_data reject source >= target && source != 0.
   • So a file containing (5, 5), whether hand-edited or from another tool, loads cleanly. It then forces every later honest attestation with source < 5 to SourceRegressed. That is exactly the wedge the new check is meant to prevent.
   • Suggest using the same predicate on read (source >= target && source != 0).

2. The rollback can leave an undecryptable keystore (http_api/keystores.rs, undo_import).

   • For a replaced entry, the keystore and the password file are each kept or removed independently, depending on whether their path matches the restored entry.
   • Consider an entry whose keystore path equals the written path but whose password path differs. The rewritten keystore stays, possibly encrypted under the new password. The new password file is removed, and the restored entry points at the old password.
   • Result: the restored key fails to decrypt at the next start.
   • Either treat the pair atomically (keep both only if both paths match), or restore the original keystore bytes captured before the import.

3. undo_import can delete a file it did not create.

   • For added keys, the rollback removes {validators_dir}/{pubkey}.json and {secrets_dir}/{pubkey} unconditionally.
   • If an orphan file already sat at either path (present on disk but not referenced by definitions), persist_import overwrote it and the rollback now deletes it.
   • This is minor, but snapshotting existence before persisting would make "removes only what this request wrote" actually true.

4. A filesystem that rejects directory fsync permanently disables signing.

   • Some FUSE, CIFS or overlay setups return EINVAL from fsync on a directory, so sync_directory fails on every write.
   • Every slot then returns Unflushed. The records advance in memory but nothing is ever signed, and startup gives no hint why.
   • Consider probing with one durable write at open_locked time, even when the file already exists and no key is missing. That would turn this into a startup error instead of silent, permanent missed duties.

5. The keymanager DELETE could now export real history.

   • The record is already a v5 minimal interchange, so returning the deleted key's entry with the real genesis_validators_root is nearly free.
   • That is much safer for migrations than the deliberately all-zero empty interchange.
   • The docs now justify the zero root as "since it does not export it yet". Worth a follow-up issue at minimum.

Rust / readability

6. A doc comment is attached to the wrong item (slashing_protection/mod.rs, around RECORD_WAIT).

   • The long blocking doc comment ("Run operation on the record on tokio's blocking pool…") runs straight into the RECORD_WAIT doc lines.
   • Rustdoc therefore attaches all of it to RECORD_WAIT, and pub async fn blocking ends up undocumented.
   • Move the two consts above the blocking doc block.

7. blocking holds a blocking-pool thread in a poll loop.

   • The try_lock loop sleeps 5 ms per attempt for up to 3 s, so each waiter occupies a pool thread for that whole time.
   • A cleaner shape is a tokio::sync::Mutex acquired with tokio::time::timeout(RECORD_WAIT, lock()) on the executor, with only the I/O inside spawn_blocking (the operation could take an owned guard). It works as is, so this is optional.

8. Check that libc is a dependency of the validator crate.

   • libc::geteuid (in attestation.rs and the slashing_protection tests) and libc::fsync (macOS branch of secure_fs.rs) are used.
   • The Cargo.toml hunk adds no libc entry, so confirm it is already a direct dependency (or dev-dependency for the tests), not reached only transitively.

9. std::fs::File::try_lock and TryLockError need Rust 1.89.

   • Make sure rust-version and the toolchain pin reflect that.
   • Also, on NFS flock may be emulated with fcntl, which can fail with EBADF on a read-only directory fd. That yields an Io error rather than silent success, which is the right side to fail on. The doc caveat in cli.md covers it.

Minor

• attestation.rs: the held filter emits tracing::debug! while the rest of the file uses the imported macros. Import debug for consistency.
• docs/cli.md: the slashing-protection blockquote has one very long line ("…may not honour. Unix only. A key with no record is refused at startup unless"). Rewrap it.
• check_and_record_attestations: the ? on Refusal inside the map closure is correct, but it is subtle next to the outer Result. An explicit return type on the closure would help readers.

Nothing blocking beyond items 1 and 2. Item 6 is a quick fix worth making before merge.

---

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

[github-actions]

🤖 Gemini Code Review

This PR is very well-designed and thorough. The fail-closed semantics on startup and signing paths, durable atomic file replacement (write_private_durably with file and directory fsyncs), directory flock acquired before key loading, bounded lock wait on the blocking pool, and keymanager rollback handling are all solid and well-covered by tests.

One minor suggestion:

1. Early return when no candidate duties remain (crates/validator/src/attestation.rs:170-195):
   If all duties in a slot have invalid pubkeys or belong to keys no longer held by store, candidates will be empty. Returning early when candidates.is_empty() before calling slashing_protection::blocking avoids an unnecessary spawn_blocking + mutex acquisition on self.protection and a second store.read().await:

   if candidates.is_empty() {
       return Ok(Attested {
           published: 0,
           refused: 0,
           data: Some(data),
       });
   }

   (Similarly, filtering candidates directly against store.read().await.contains(pubkey) would avoid allocating a Vec<BlsPubkey> and HashSet<BlsPubkey> of all loaded keys on every slot.)

---

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

[pablodeymo]

Fourth automated round, at 11f8814. Fixed in b53ac02 unless noted.

• Read and write paths disagree on source == target (Claude 1): record_of now uses the same predicate as the record and the attestation-data check, so a file carrying such an entry stops the client at startup naming it. Test added.
• Rollback can leave an undecryptable keystore; it can delete a file it did not create (Claude 2, 3): persist_import snapshots the bytes at both paths before writing, and an undone import puts back exactly that, as a pair, or removes the files where there was nothing. Test added for the re-import case.
• A filesystem that refuses directory fsync disables signing silently (Claude 4): the record is written once at every start, whatever its state, so that becomes a startup error naming the path.
• Delete retries hide a non-durable removal (Codex P1): an unflushed removal sets a flag the keymanager keeps until a later save flushes the directory; a retry saves the file again before answering not_found, and keeps the error until the flush works. No injection test, since a directory flush cannot be made to fail portably; the path is three lines behind a flag.
• Two idle epochs (Codex P1): the README and the startup error carry the surround caveat.
• Early return on no candidates (Gemini): done; the set allocation stays, it is a few dozen bytes a slot.
• DELETE could export real history (Claude 5): feat(validator): EIP-3076 slashing-protection import and export #736.

[github-actions]

🤖 Grok Code Review

The core rule is sound: record-before-sign, fail-closed reads, directory flock instead of a deletable lock file, and registration rollback are the right shape. A few boundary cases can still lose keys or wedge them.

1. Import drops a provided interchange and starts the key as never-signed. In crates/validator/src/http_api/keystores.rs a POST with slashing_protection set only logs a warning and continues. That is the migration path most likely to double-sign: the caller handed over history and this client throws it away. Until import works, reject the request when that field is present. Same gap on the way out: file.rs says the on-disk bytes are the export format, but DELETE still returns an empty all-zero interchange, so history this process actually recorded cannot be moved safely.

2. One bad attestation response can pin the source watermark forever. validate_attestation_data only rejects source >= target (except genesis). A source that is still < target but above the chain’s justified checkpoint is recorded, and every later honest attestation is SourceRegressed. During non-finality that may never heal, and the only recovery is hand-editing slashing_protection.json. Before check_and_record_attestations, refuse data whose source is ahead of this beacon node’s own justified checkpoint (same node, inconsistency check). A single malicious node can still lie on both endpoints; a buggy one usually cannot.

3. Error::Unflushed on delete does not match the comment, and may desync the API from the signer. delete_one returns an error before the store removal, so this process keeps signing. The comment says it does not. If persist_delete has already dropped the definitions entry, GET /eth/v1/keystores can omit a key that is still signing. An operator who trusts the list and imports elsewhere will slash. On Unflushed, either roll the definitions mutation back so the key stays listed, or stop signing and make the error impossible to treat as success. Do not remove it from the store while the on-disk removal might not survive power loss.

4. A failed publish after a successful record is now an epoch-long miss, and the duty loop still does not resubmit. attest and propose both say the caller must resubmit the already-built batch, and both note that today’s loop does not. Re-signing is correctly refused. Retain the signed attestation batch and the signed block and retry publication only; do not call attest / propose again.

5. commit’s Err is not “nothing changed”. On Error::Unflushed, commit extends in-memory watermarks and then returns Err. check_and_record_attestations documents the opposite. Callers that treat every Err as “do not sign” are safe; do not “fix” them by signing on Unflushed, or by assuming a retry would see the old watermark.

Minor: the sentences about RECORD_WAIT were left inside blocking’s doc comment. file::record_of rejects source > target but accepts source == target != 0, which the signer itself will not write; reject that at load too. If directory fsync always fails, an existing file lets the process start and then refuse every signature — probe a durable write at startup. The unconditional startup warning is gone; a short one that cross-machine use and history import are still uncovered is still worth saying out loud.

The minimal watermark rule, slot 0 / epoch 0 handling, restart tests, and symlink/0600 temp-file handling look right.

---

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

[github-actions]

🤖 DeepSeek Code Review

⚠️ Review failed: DeepSeek returned an empty content field (finish_reason=length, reasoning_content present=yes). Raise max_tokens or lower reasoning_effort ('none' disables thinking).

---

Automated review by DeepSeek

[github-actions]

🤖 Kimi Code Review

The core implementation looks strong: watermarks follow EIP-3076’s minimal rule, reads fail closed, the directory lock survives record renames, and recording happens before signing. Restart, duplicate-duty, failed-write, and keymanager rollback coverage is particularly good.

1. Keymanager imports still discard supplied slashing history and report success
   crates/validator/src/http_api/keystores.rs:145-151, with registration around lines 230-260. A POST containing slashing_protection only produces a server-side warning; the key is subsequently registered as never having signed and loaded. A caller migrating an active key through the standard API can therefore receive imported and immediately produce a double or surround vote. Until history import is implemented, reject requests carrying that field without persisting, registering, or loading their keys. Silently ignoring other optional fields is reasonable, but this one directly controls slashing safety. The documentation acknowledges the limitation, but the API response should also fail closed.

2. The error contract is inconsistent for Unflushed writes
   In crates/validator/src/slashing_protection/mod.rs, check_and_record_block and check_and_record_attestations document that an outer Err means “nothing changed,” and the error branch in crates/validator/src/attestation.rs says “Nothing was recorded.” However, commit deliberately extends self.records when file::write returns Error::Unflushed, after the file has already been replaced. Current signing behavior remains safe, and the keymanager handles this exception explicitly, but the public method contract is misleading and could encourage incorrect rollback or retry logic in future callers. Document the exception on both methods and correct the attestation comment, or return a typed outcome distinguishing “not recorded” from “recorded visibly, but directory durability is uncertain.”

3. Minor: the strict file reader accepts an attestation shape the signing path rejects
   crates/validator/src/slashing_protection/file.rs, in record_of, rejects only source_epoch > target_epoch. Elsewhere, equal nonzero source and target epochs are considered invalid. Accepting such history is conservative rather than unsafe, but it is inconsistent with the fail-closed/strict-reader policy. Consider rejecting source_epoch == target_epoch unless both are zero.

---

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

[pablodeymo]

On Grok's points at b53ac02.

• commit's Err is not "nothing changed" (5): right, and the docs of check_and_record_block and check_and_record_attestations now say so (c2f45b3): on Unflushed the slot or target is on disk and in memory and a retry is refused; every Err means do not sign. Callers already treat it that way.
• Delete keeps signing on Unflushed (3): the store removal happens before persist_delete (delete_one, lines 707 and 751), so the key is out of memory before the file is touched; GET lists from the store, so it is omitted because it no longer signs. Rolling the file back would keep a removed key signing, the unsafe direction. The error says the removal may not survive a power loss, and since b53ac02 a retry flushes before answering not_found.
• Import drops the interchange; DELETE exports nothing (1): feat(validator): EIP-3076 slashing-protection import and export #736, stacked on this PR.
• Source pinned by one bad response (2): checking the source against the same node's justified checkpoint costs a request per slot and only catches a node that disagrees with itself; it is documented as the remaining trust. A cross-check against another node belongs with doppelganger protection.
• Failed publish is an epoch-long miss (4): publication already goes through every configured node via the fallback client, and re-signing is refused by design. Retrying publication of the retained signed messages is a follow-up.

[pablodeymo]

On Kimi's points.

• Imports discard supplied history (1): feat(validator): EIP-3076 slashing-protection import and export #736, stacked on this PR, imports slashing_protection in the same durable write as the registration and refuses the whole request when it is unusable; DELETE there exports the real record.
• Unflushed contract (2): the docs of check_and_record_block and check_and_record_attestations state the exception since c2f45b3, and the "nothing was recorded" comment in attestation.rs matches since 4a31fc4. The typed outcome you suggest is what Error::Unflushed is: callers treat every Err as do-not-sign, and only the keymanager reads the variant.
• Reader accepts source == target (3): fixed in b53ac02, with a test; it is refused unless both are zero, the same predicate as the record and the attestation-data check.

[pablodeymo]

/review

[github-actions]

🤖 Codex Code Review

No actionable findings. The watermark rules, durable record-before-sign ordering, and handling of failed writes look sound.

Tests could not run: the pinned Rust toolchain was unavailable, and the installed toolchain lacked the cached libssz Git dependency required for offline testing.

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 734, durable EIP-3076 minimal slashing protection

This is a solid, carefully reasoned PR. The ordering is right: check, then durable record, then sign. Reads fail closed, the lock sits on the directory inode, and the Unflushed handling keeps in-memory state at least as strict as what is on disk. I found one real bug in the import rollback path. The rest are smaller.

Bugs / correctness

1. undo_import restores files in the wrong order when one request imports the same key twice (http_api/keystores.rs, undo_import).

   • persist_import captures Overwritten just before each write. The second copy of a key therefore records the first copy's freshly written bytes as "previous".
   • files is iterated forward. The original bytes get restored first, then the second entry writes the first import's keystore and password back over them.
   • Result: after a failed registration, the new keystore is left on disk. In the "replaced" case, the restored definitions entry may point at it.
   • Fix options:
     • iterate files in reverse (files.iter().rev()), or
     • dedupe pubkeys in the request up front. Rejecting duplicates as an error is simplest.

2. Doc comment attached to the wrong item (slashing_protection/mod.rs, around blocking).

   • The long /// Run operation on the record on tokio's blocking pool… block sits directly above const RECORD_WAIT, followed by RECORD_WAIT's own doc.
   • Rustdoc merges all of it onto RECORD_WAIT, so blocking ends up undocumented.
   • Fix: move the consts above that doc block.

3. libc dependency (secure_fs::sync_directory).

   • On macOS, non-test code calls libc::fsync. Tests in attestation.rs and slashing_protection call libc::geteuid.
   • The Cargo.toml diff doesn't add libc. If it isn't already a direct dependency of ethlambda-validator, the macOS build and the unix tests won't compile.
   • Please confirm it is declared, and not just reachable transitively.

4. MSRV: File::try_lock and std::fs::TryLockError were stabilized in Rust 1.89. Make sure rust-version / the toolchain pin reflects that.

Design / safety observations

5. DELETE /eth/v1/keystores still returns an empty interchange with a zero root.

   • The on-disk record is now already an EIP-3076 v5 minimal interchange, and records survive key deletion.
   • So exporting the deleted keys' entries with the real genesis_validators_root is nearly free.
   • Until that lands, a key migrated out through the API leaves its watermarks behind. That is exactly the scenario the new startup refusal exists to catch on the receiving side.
   • Worth a follow-up issue at minimum.

6. blocking's try_lock polling is unfair.

   • Under sustained contention, for example a keymanager import loop while a slot's attestation batch waits, a waiter can lose every race and hit the 3 s refusal even though no write is stuck.
   • In practice contention is low, so this is acceptable.
   • Consider making the refusal message not assert "may be stuck" so confidently, or note the possibility in the doc.

7. deny_unknown_fields on Interchange, Entry and Metadata (file.rs).

   • This is correct for the client's own file.
   • When import support lands, it should not reuse these types as-is. Other clients' interchange files may carry fields this strict reader would reject.
   • A short comment would prevent that reuse.

8. file::exists treats a symlinked record as fatal.

   • This is defensible fail-closed behaviour. Operators who symlink slashing_protection.json onto another volume will hit it, though.
   • docs/cli.md should mention it alongside the read-only-mount note.

Minor

• attestation.rs: the held snapshot allocates a HashSet from pubkeys() every slot. That's fine at hundreds of keys. If the store can answer contains directly, use it.
• Attested.refused doesn't count keys that failed at signing because they became unknown after the snapshot. That is consistent with the field's doc, so this is just a note.
• validate_attestation_data: the new source >= target && source != 0 check duplicates Record::check_attestation and file::record_of. A small shared helper (fn is_impossible_ffg(source, target) -> bool) would keep the three in sync.

Tests

Coverage is good:

• restart refusal for both blocks and attestations
• unwritable directory
• duplicate keys within a batch
• damaged files
• symlinked record
• second-process lock
• import rollback, including restoring overwritten files

Missing: a regression test for Item 1, i.e. the same keystore twice in one POST with registration failing.

---

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

[github-actions]

🤖 Gemini Code Review

Solid implementation of durable EIP-3076 minimal-watermark slashing protection. The fail-closed design (strict file parsing, directory flock acquired before loading keys, durable write-then-sync before signing, in-memory watermark advancement on Error::Unflushed, and keymanager rollback on failed registration) is well thought out and thoroughly tested.

A couple of minor issues spotted:

1. crates/validator/src/http_api/keystores.rs (lines 295–304, 392–399) — undo_import restores the wrong file state if a batch contains duplicate pubkeys:
   prepare_import runs sequentially for each entry in the batch and snapshots regular_file_bytes right before writing. If a single import request lists the same pubkey twice, the first entry captures the true pre-request state (overwritten_1), while the second entry captures the bytes just written by the first (overwritten_2). Because undo_import iterates files in forward order:

   for (pubkey, overwritten) in files {
       let [keystore, password] = written_paths(context, pubkey)?;
       put_back(&keystore, overwritten.keystore.as_deref())?;
       put_back(
           &password,
           overwritten.password.as_deref().map(|bytes| &bytes[..]),
       )?;
   }

   overwritten_2 is applied after overwritten_1, leaving the first entry's keystore and password on disk (or overwriting the pre-existing files with them) instead of restoring the pre-request state. Iterating files.iter().rev() in undo_import ensures the earliest snapshot for any duplicated pubkey wins.

2. crates/validator/src/slashing_protection/mod.rs (lines 81–106) — Misplaced doc comment on blocking:
   RECORD_WAIT and RECORD_POLL are placed between the doc comment for pub async fn blocking and the function definition itself, so rustdoc attaches the entire doc comment to the private RECORD_WAIT constant and leaves blocking undocumented. Moving RECORD_WAIT and RECORD_POLL above the /// Run operation... doc block fixes this.

---

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

[pablodeymo]

Sixth automated round, at 4a31fc4. Fixed in d2d495f unless noted.

• undo_import with the same key twice in one request (Claude 1, Gemini 1): right, and both of you caught it. The undo now applies snapshots newest first, so the earliest one lands last; a regression test imports the same keystore twice with the registration failing.
• Doc comment attached to RECORD_WAIT (Claude 2, Gemini 2): the constants moved above the block; blocking is documented again.
• Three copies of the source-at-target predicate (Claude, minor): one is_impossible_source now, used by all three.
• try_lock polling is unfair (Claude 6): the refusal no longer asserts the write is stuck; a busy keymanager is named as the other cause.
• deny_unknown_fields and other clients' files (Claude 7): a comment on the types says they are for this client's own file only; feat(validator): EIP-3076 slashing-protection import and export #736's importer is the one that reads other clients' files.
• Symlinked record is fatal (Claude 8): now in docs/cli.md beside the read-only-mount note.
• Already answered in earlier rounds, unchanged: libc is declared under [target.'cfg(unix)'.dependencies]; the workspace pins Rust 1.97.1; DELETE exports the real record in feat(validator): EIP-3076 slashing-protection import and export #736; the held-keys set is a few dozen bytes a slot; Attested.refused counts refusals by the record, as its doc says.

[github-actions]

🤖 Grok Code Review

Slashing protection is carefully ordered (durable record, then sign) and fails closed on a bad file, a second process, and a source that would wedge the watermark. A few gaps are still worth fixing.

1. Attested.data is Some even when nothing was signed (crates/validator/src/attestation.rs, the write-failure return and the attestations.is_empty() return). The field is documented as data that was signed. A duty loop that aggregates whenever data is Some will aggregate a slot this validator refused (record write failed, or every duty refused). Return None when published == 0, or gate aggregation on published > 0.

2. Keymanager still throws away history (crates/validator/src/http_api/keystores.rs). POST ignores slashing_protection, and DELETE still returns an empty interchange with a zero genesis root. The on-disk format is already a minimal EIP-3076 file, and record_of already reduces multiple entries to a stricter max watermark. Importing that max (and exporting the live watermarks with the real genesis root on DELETE) would close the migration double-vote this PR still documents. As written, a standard keymanager move into this client starts the key as never having signed.

3. Partial keystore write is not undone (persist_import in keystores.rs). Overwritten is snapshotted, then the keystore is replaced, then the password is written. If the password write fails, prepare_import returns Err, the key is not in persisted, and undo_import never runs. A re-import can destroy the previous keystore and leave the old password/definitions in place. Roll back with Overwritten on that path too.

4. A bit-flip can lower a watermark and still parse. file::read fail-closes on empty, truncated, or unknown JSON, but a flipped digit in "slot" / "target_epoch" is still valid and is loaded. That is a silent downgrade of the only slash record. A checksum (or a monotonic generation counter) that fails closed would match the rest of the design. Also, open_locked always rewrites the file, so any signing_root or extra history an operator dropped in is collapsed on the next start.

5. Lock wait vs write timeout (slashing_protection/mod.rs, RECORD_WAIT). The 3s cap is only on try_lock. The fsync itself is unbounded, and a timeout becomes ProposalRefused, which is not retryable, so a slow attestation flush can drop a proposal that was never recorded. Either bound the write or let a lock-timeout proposal be retried within the slot.

delete correctly leaves watermarks in place, register does not lower them, and source >= target (except genesis (0, 0)) is rejected before it can pin every later attestation. That part is right. file should be pub(crate); the unix tests call libc::geteuid — confirm libc is a direct dev-dependency or those tests will not build.

---

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

[github-actions]

🤖 DeepSeek Code Review

The PR is substantial and the overall design is sound, with good fail-closed behavior and extensive tests. A few issues worth addressing:

• Failed import can corrupt existing keystore/password files. In crates/validator/src/http_api/keystores.rs, persist_import captures the previous file bytes in Overwritten, but only undo_import restores them on registration failure. If secure_fs::write_private_no_symlink for the password fails, or save_definitions returns a non-Unflushed error after both files were written, persist_import returns Err without using Overwritten. For a re-import of the same pubkey, the old definitions entry still points at the same paths, so the next start loads the newly written keystore/password even though the import was reported as failed. The fix is to restore/remove those files inside persist_import on every error path after the first file write.

• ValidatorDefinitions::save is now a hard failure on non-Unix. It uses secure_fs::write_private_durably, whose non-Unix implementation returns ErrorKind::Unsupported before touching the file. If this crate is built on Windows or another non-Unix target, every keymanager definitions save will fail—not just slashing protection. If the client is intentionally Unix-only this should be clearly documented/compiled out, otherwise a non-Unix fallback is needed.

• Keymanager import silently registers keys with no history. In the import handler, every persisted key is registered via protection.register(&persisted), regardless of whether the request carried slashing_protection history. This is exactly the same as --init-slashing-protection but without the explicit startup gate. A key with real signing history elsewhere could be imported with no record and immediately used, risking a double vote/surround. Consider requiring an explicit API-level opt-in for unknown keys, or refusing keys with no record unless a valid EIP-3076 history is supplied, until history import is supported.

• Minor: slashing_protection::blocking's 3-second timeout only bounds lock acquisition, not the duration of the guarded operation. Once the lock is acquired, a stuck file::write/fsync can hang indefinitely on that blocking thread. The outer duty deadline may drop the future, but the blocking operation continues and can pin a blocking-pool thread. This is probably acceptable given the fail-closed design, but worth noting in the module docs or metrics.

---

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

[pablodeymo]

On Grok's round at d2d495f. Fixed in b339ff7 unless noted.

• Attested.data is Some when nothing was signed (1): by design, and now said in the field's first sentence: it is the slot's data, which the aggregation duty needs whatever became of this client's own signatures; published and refused are the record of what was signed.
• Keymanager throws history away (2): POST now refuses a request carrying slashing_protection whole, importing none of its keys, so a migration that hands over history fails loudly. DELETE keeps the deliberately unusable empty interchange. feat(validator): EIP-3076 slashing-protection import and export #736, stacked on this PR, imports and exports the real thing.
• Partial keystore write is not undone (3): right. persist_import now puts both files back when the second write, or the entry's save, fails; a test makes the password write fail and checks the old keystore is back.
• A bit flip can lower a watermark; the startup rewrite collapses extra history (4): both by design, now written down in spec_deviations.md: the file is an interchange other tools can read and the operator can edit, which the documented recovery relies on, and a checksum would make it a private format that refuses that edit. The startup rewrite in the minimal shape is stated too.
• Lock wait versus write timeout (5): the blocking doc now says a duty refused after the wait is not retried within the slot, since the holder is a flush a retry would wait on as well; nothing was recorded for it, and the write itself cannot be bounded, only outwaited.

[github-actions]

🤖 Kimi Code Review

1. Security: runtime key imports bypass the fail-closed initialization policy — crates/validator/src/http_api/keystores.rs, around the slashing_protection warning and subsequent register call. An import carrying EIP-3076 history has that history ignored, yet new keys are registered as never-signed, loaded, and reported as imported. Startup requires the explicit --init-slashing-protection opt-in for the same operation, but the keymanager path does not. An operator importing an active key can therefore double-sign despite supplying its history. Until history import is supported, consider rejecting such imports, or requiring an explicit unsafe-registration option rather than silently succeeding.

2. Rollback restores file snapshots in the wrong order for duplicate pubkeys — keystores.rs, undo_import, particularly for (pubkey, overwritten) in files. Nothing shown deduplicates keystores by pubkey. If one request imports the same key twice, the second Overwritten snapshot contains the files written by the first import. Restoring snapshots in forward order leaves that intermediate version on disk instead of the original files—or leaves orphan files where none originally existed. Restore file snapshots in reverse order (files.iter().rev()) and add a duplicate-pubkey rollback test.

3. An after-rename flush failure aborts rollback with definitions and files inconsistent — keystores.rs, undo_import’s save_definitions(context, definitions)?. Error::Unflushed means the restored definitions are already visible; only durability is uncertain. Returning immediately skips restoration of every keystore/password file, potentially leaving restored definitions pointing at the newly written files. Rollback should continue restoring files best-effort on Unflushed, then report the durability error afterward.

The core signing path otherwise looks sound: checks and durable recording happen before signatures exist, write failures fail closed, persisted records survive restarts, and the block/attestation watermark rules are well covered by tests.

---

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

[MegaRedHand]

Re-reviewed at b339ff7. Everything from the previous round is in. One small thing inline.

[MegaRedHand]

Since b339ff7, undo_import also puts this key's keystore and password files back, so "keystore rewritten" no longer describes what happened when the rollback succeeds. Something like "not imported: its slashing-protection record could not be written: {err}; its keystore and definitions entry are back as they were" would match, keeping the current wording for the rolled_back.is_err() case.

[MegaRedHand]

LGTM

[MegaRedHand]

This using ethlambda-validator just for test is strange