[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 if it had signed a block and an attestation at the current slot and epoch, so it costs at most the rest of the epoch. It is still meant only for keys that have signed nothing anywhere for at least two epochs, and it never lowers an existing record.

• 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"	--init-slashing-protection and keymanager imports now seed the current slot and epoch (block slot = now, attestation target = current epoch). The source stays at 0: seeding it higher would refuse every attestation until justification caught up
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.