[MegaRedHand]

🗒️ Description / Motivation

Without --node-key, both sub-commands generated a fresh secp256k1 key in memory on every start, so the node's PeerId and ENR changed on each restart. A lean node lost its place in every peer's scoring. On beacon it was worse: the custody columns are a function of the node id, so after a restart the sidecars already on disk belonged to a custody set nobody would select again, and the new id had nothing custodied for it yet. Startup warned about both, and the README told operators to generate a key with openssl to avoid it.

This PR makes the node keep its own identity: when --node-key is absent, it stores a generated key in --data-dir and reads it back on every later start.

What Changed

Area	Change
bin/ethlambda/src/main.rs	resolve_node_key(cli, data_dir): --node-key if given, else <data-dir>/node.key, generating and storing it on the directory's first start. Resolution moves below the RocksDB open. New read_node_key validates the bytes as a secp256k1 key
bin/ethlambda/src/beacon.rs	wire_params drops its node_key_supplied parameter and the custody warning it gated
crates/validator/src/secure_fs.rs	secure_fs module and write_private go from pub(crate) to pub, so the binary writes the key with the same 0600 helper the validator client uses for its secrets
bin/ethlambda/src/cli.rs, docs/{cli,architecture,data_storage}.md, CLAUDE.md	Describe the new default and the new startup order
README.md	The quick starts no longer generate a key with openssl or pass --node-key

Correctness / Behavior Guarantees

--node-key given?
├─ yes → read it; nothing is written, a stored node.key is ignored
└─ no  → <data-dir>/node.key exists?
         ├─ yes → read it; a file that fails to load is a startup error, never replaced
         └─ no  → generate, write node.key.tmp (0600), rename to node.key, use it

• The stored key is never regenerated. A stored file that fails to load (bad hex, wrong length, not a valid secret key) is a startup error naming the path, since replacing it would silently discard an identity that was working.
• A supplied --node-key wins. Passing it on a directory that already holds node.key uses the supplied key and leaves the stored one untouched.
• Both paths now validate the key at load. An invalid --node-key used to reach build_swarm and panic with invalid node key; now it is a startup error naming the file.
• Same format as --node-key. The stored file is hex with a trailing newline, so it can be passed as --node-key to another run.
• Written under RocksDB's lock. Resolution runs after RocksDBBackend::open, so two processes started on one directory cannot both generate a key and race to store it. The .tmp + rename means an interrupted first start leaves no partial key.
• The key sits in RocksDB's own directory. RocksDB ignores files it did not write; rocksdb_reopens_a_data_dir_holding_a_stored_node_key pins that. Wiping the directory resets the identity together with the custody columns it selected.
• The custody warning in beacon::wire_params is removed because the identity is now always stable across restarts.
• The Hive test-driver early return still comes before key resolution, so a driver run never touches the data directory.

Tests Added / Run

Added in bin/ethlambda/src/main.rs:

• a_supplied_node_key_is_read_verbatim_and_nothing_is_stored
• a_missing_node_key_is_generated_once_and_reloaded_from_the_data_dir (also checks that the stored file reads back as --node-key)
• a_generated_node_key_is_stored_at_mode_0600 (unix)
• a_stored_node_key_that_fails_to_load_is_an_error_not_a_new_key
• rocksdb_reopens_a_data_dir_holding_a_stored_node_key

Run:

• cargo test -p ethlambda --profile release-fast --bins
• cargo test -p ethlambda-validator --profile release-fast --lib

Related Issues / PRs

None.

✅ 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) — not run in full; only the two touched crates' unit tests above

[github-actions]

🤖 Kimi Code Review

I'll review this PR which changes node key handling from ephemeral in-memory generation to persistent storage in the data directory. Let me analyze the changes across all files.

Security Review

Critical: Race Condition in Key Generation (bin/ethlambda/src/main.rs:1350-1355)

let generated = secp256k1::SecretKey::new(&mut secp256k1::rand::rngs::OsRng).secret_bytes();
let contents = format!("{}\n", hex::encode(generated));
let temporary = data_dir.join(format!("{STORED_NODE_KEY_FILE}.tmp"));
ethlambda_validator::secure_fs::write_private(&temporary, contents)
    .and_then(|()| std::fs::rename(&temporary, &stored))

The comment states "RocksDB's lock is what stops two processes started on it from both generating a key and racing to store theirs." However, RocksDB's file lock does NOT prevent concurrent access to other files in the same directory. Two processes can still race:

1. Process A checks node.key doesn't exist
2. Process B checks node.key doesn't exist
3. Both generate keys
4. Both write to .tmp files (different PIDs, but same race)
5. rename() is atomic, but the loser overwrites the winner

The .tmp suffix uses a fixed name, not PID-based, so two processes will collide on the temporary file itself. More critically, the existence check and write are not atomic.

Fix: Use an atomic create-if-not-exists operation, or better, use O_CREAT|O_EXCL on the final file. The current write_private doesn't support this.

High: Missing Error Context Chain (bin/ethlambda/src/main.rs:1345-1348)

let exists = stored
    .try_exists()
    .wrap_err_with(|| format!("failed to look for a node key at {}", stored.display()))?;

try_exists() can return Ok(false) for permission errors (where the parent directory exists but isn't readable). This is a subtle footgun - the code treats "definitely doesn't exist" and "can't tell if it exists" identically. On Unix, ENOENT vs EACCES should be distinguished.

Correctness Issues

Medium: Hex Format Inconsistency (bin/ethlambda/src/main.rs:1350)

let contents = format!("{}\n", hex::encode(generated));

The stored format adds a newline, but read_hex_file_bytes likely strips whitespace. Verify this is consistent - if read_hex_file_bytes doesn't handle newlines, the reload will fail. The test a_missing_node_key_is_generated_once_and_reloaded_from_the_data_dir suggests it works, but this is an implicit dependency.

Low: read_node_key Validation vs Usage Mismatch (bin/ethlambda/src/main.rs:1367-1372)

fn read_node_key(path: &Path) -> eyre::Result<Vec<u8>> {
    let key = read_hex_file_bytes(path)?;
    secp256k1::SecretKey::from_slice(&key)
        .wrap_err("the file does not hold a valid secp256k1 secret key")?;
    Ok(key)
}

The function validates the key but returns the raw bytes, not the SecretKey. This is correct but fragile - the validation is side-effect only. Consider returning SecretKey to ensure callers can't accidentally use unvalidated bytes.

Rust Best Practices

Minor: format! in const Context (bin/ethlambda/src/main.rs:1349)

let temporary = data_dir.join(format!("{STORED_NODE_KEY_FILE}.tmp"));

This allocates for a fixed suffix. Prefer:

const STORED_NODE_KEY_FILE: &str = "node.key";
const STORED_NODE_KEY_TMP: &str = "node.key.tmp";

Minor: Inefficient Double-Read on Supplied Key (bin/ethlambda/src/main.rs:1333-1338)

if let Some(path) = node_key_path {
    let key = read_node_key(path)
        .wrap_err_with(|| format!("failed to load node key from {}", path.display()))?;
    info!(path = %path.display(), "Loaded the node key from --node-key");
    return Ok(key);
}

The read_node_key function reads the file, then read_hex_file_bytes reads it again internally? No, looking again - read_node_key calls read_hex_file_bytes. This is fine, single read.

Minor: Test Helper Naming (bin/ethlambda/src/main.rs:3294)

The test rocksdb_reopens_a_data_dir_holding_a_stored_node_key is good but could be clearer about testing the coexistence, not just reopening.

Documentation & Maintainability

Positive: Excellent Documentation

The CLAUDE.md, README.md, and inline comments are thorough. The architecture docs correctly reflect the new ordering (DB open before key resolution).

Minor: Comment Stale Reference (bin/ethlambda/src/beacon.rs:202-204)

// `--node-key`, or the key `resolve_node_key`
// stored in the data directory beside the columns it custodies.

The function name reference is slightly awkward. Consider: "or the key stored in the data directory by resolve_node_key".

Testing

Positive: Comprehensive Test Coverage

The tests cover:

• Supplied key reading without storage side effects
• Generation and reload
• Cross-compatibility between stored file and --node-key flag
• Unix permissions (0600)
• Error handling for corrupted stored keys
• RocksDB coexistence

Minor: Missing Concurrent Access Test

No test for the race condition mentioned in the security section. Given the claim that RocksDB's lock prevents races, there should be a test attempting concurrent key generation.

Consensus-Layer Impact

No direct consensus code changes. The custody column computation remains correct since node_id is still derived the same way. The behavioral change is purely operational - identity stability across restarts, which is critical for DAS (Data Availability Sampling) correctness:

• Before: Ephemeral identity → changing custody columns → stored sidecars become orphaned
• After: Stable identity → consistent custody columns → sidecars remain valid

This is a significant improvement for peer DAS functionality.

Summary

Severity	Issue	Location
Critical	Race condition in key generation despite RocksDB lock claim	main.rs:1340-1355
High	try_exists() conflates "doesn't exist" with "permission denied"	main.rs:1345-1348
Medium	Implicit newline handling in hex format	main.rs:1350
Low	read_node_key returns raw bytes after validation	main.rs:1367-1372

The core design is sound and well-documented, but the race condition needs addressing before merge. Consider using std::fs::OpenOptions::new().create_new(true) or a proper file lock for the key file itself.

---

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

[github-actions]

🤖 Claude Code Review

Review of PR 726: store a generated node key in the data directory

I read resolve_node_key and read_node_key in bin/ethlambda/src/main.rs, plus write_private in crates/validator/src/secure_fs.rs. I did not run the tests. The checked-out HEAD commit contains the whole repo, so I could not isolate the PR diff and may have missed changes outside these two functions.

The design is sound. Key resolution runs after the RocksDB open, so the DB lock serializes first-start generation. A stored file that fails to load is an error rather than being replaced. The key is validated at load, so a bad key no longer panics in build_swarm. The file is written at mode 0600 and renamed into place from a .tmp sibling.

Findings

1. Stale comment (bin/ethlambda/Cargo.toml, secp256k1 dependency): it still says "ephemeral-key generation when --node-key is omitted". The key is now persistent, so update it.

2. No fsync before the rename (main.rs:1359-1360): after a power loss the rename can survive while the file data does not. The next start would then find an empty or truncated node.key and refuse to start, because the stored file is never replaced. This is rare, and failing loudly beats silently changing identity. You could still sync_all() the temp file before renaming, which means writing it without write_private.

3. Stale .tmp file (main.rs:1358): write_private truncates and reuses an existing node.key.tmp, so a leftover from a crash is harmless. The one gap is an existing .tmp created with a looser mode. mode(0o600) applies only on creation, so a leftover file keeps its old mode. The key would then be written into a world-readable file before the rename. A remove_file of the temp path first, ignoring NotFound, would close it. The risk is low.

4. Non-unix targets: write_private falls back to std::fs::write with no mode restriction. That is acceptable, but worth a doc note.

5. Key error handling (main.rs:1371-1376): read_node_key keeps the decoded bytes in a plain Vec<u8>, and the SecretKey it builds for validation is dropped without being zeroized. This matches the existing pattern, so it is not a blocker.

6. Tests: they cover reuse, 0600 mode, an invalid file, --node-key not writing anything, and a RocksDB reopen. I'd add one for a path where node.key exists but is a directory or unreadable, to confirm the error path rather than a regenerate. That follows from the "never replace" rule.

Overall this is good to merge once the stale comment (Item 1) is fixed. The fsync and .tmp mode points (Items 2 and 3) are optional hardening.

---

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

[github-actions]

🤖 Codex Code Review

• Overall this looks solid. The change is operational, not consensus-critical: it persists the libp2p/discv5 identity under --data-dir, and the added tests cover the happy path, invalid stored key handling, file mode, and RocksDB reopen behavior.

• One security concern: resolve_node_key writes via write_private to node.key.tmp, then renames it into place, but write_private does not use O_NOFOLLOW. A local attacker who can place node.key.tmp as a symlink inside the data dir before first startup can redirect the write to an arbitrary file writable by the operator. This is exactly the class of issue write_private_no_symlink already prevents elsewhere. Suggest using the no-symlink variant for the temp file too, or otherwise opening the temp path with O_NOFOLLOW. See bin/ethlambda/src/main.rs:1358 and crates/validator/src/secure_fs.rs:26.

• Related race/correctness point: the “RocksDB lock covers the write” argument is only as strong as “other ethlambda processes also open the same DB first”. If two unrelated local processes touch the directory before the node does, the temp-file symlink issue above still applies. Fixing Point 1 closes the meaningful gap.

• Minor maintainability concern: crates/validator/src/lib.rs:46 makes secure_fs fully public just so the binary can call one helper. If this crate is published or consumed externally, that broadens API surface unnecessarily. Prefer re-exporting only the needed function or moving the helper to a more shared internal crate/module.

• I did not see issues in fork choice, attestation processing, justification/finalization, state transition, XMSS, or SSZ from this PR; those paths are not materially changed here.

---

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