diff --git a/CLAUDE.md b/CLAUDE.md index a171704a..26242e57 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -421,10 +421,14 @@ where a bad value is reported. `run_node` owns everything that is not chain-specific, in order: the discv5 port check, metrics registration, the banner and version log, the `RLIMIT_NOFILE` raise, the `HIVE_LEAN_TEST_DRIVER` early return (lean-only, and -it must still precede key loading), `--node-key` resolution, resolving -`--network` into a `NetworkSource` (mainnet only; must precede the next step, -since a loaded network's own bootnode list is part of its fallback), reading -`--bootnodes`, and building the aggregator, sync-status and event handles. +it must still precede key loading), resolving `--network` into a +`NetworkSource` (mainnet only; must precede the next step, since a loaded +network's own bootnode list is part of its fallback), reading `--bootnodes`, +building the aggregator, sync-status and event handles, opening `--data-dir`'s +RocksDB backend, and `--node-key` resolution. That last one comes after the +open because, without `--node-key`, the identity is `/node.key`, +generated and stored on the directory's first start, and RocksDB's lock is +what keeps two processes on one directory from racing to write it. It then branches **once**, on `network`, both arms inline, each evaluating to a `ChainSetup`: the wire configuration, the ENR entries that describe it, the diff --git a/README.md b/README.md index 2b3457da..c6293e5b 100644 --- a/README.md +++ b/README.md @@ -36,8 +36,6 @@ mv ethlambda-linux-x86_64 ethlambda && mv ethrex-linux-x86_64 ethrex ./ethlambda --version && ./ethrex --version mkdir -p beacon-data ethrex-data -# A persisted key keeps the node's identity, and so its custody set, stable across restarts -openssl rand -hex 32 > beacon-data/node-key # The secret both clients authenticate the Engine API with openssl rand -hex 32 > jwt.hex @@ -51,7 +49,6 @@ openssl rand -hex 32 > jwt.hex ./ethlambda beacon \ --network mainnet \ --checkpoint-sync-url https://beaconstate.ethstaker.cc \ - --node-key beacon-data/node-key \ --data-dir beacon-data/db \ --discovery.advertise-ip "$(curl -s https://ifconfig.me)" \ --execution-endpoint http://127.0.0.1:8551 \ @@ -129,8 +126,6 @@ docker pull ghcr.io/lambdaclass/ethlambda:latest docker pull ghcr.io/lambdaclass/ethrex mkdir -p beacon-data ethrex-data -# A persisted key keeps the node's identity, and so its custody set, stable across restarts -openssl rand -hex 32 > beacon-data/node-key # The secret both clients authenticate the Engine API with openssl rand -hex 32 > jwt.hex # This host's public address, published in the node's ENR (see above) @@ -159,7 +154,6 @@ docker run -d --name ethlambda-beacon --network ethereum --stop-signal SIGINT \ ghcr.io/lambdaclass/ethlambda:latest beacon \ --network mainnet \ --checkpoint-sync-url https://beaconstate.ethstaker.cc \ - --node-key /data/node-key \ --data-dir /data/db \ --http-address 0.0.0.0 \ --discovery.advertise-ip "$PUBLIC_IP" \ @@ -177,8 +171,6 @@ and no `--execution-*` flags. docker pull ghcr.io/lambdaclass/ethlambda:latest mkdir -p beacon-data -# A persisted key keeps the node's identity, and so its custody set, stable across restarts -openssl rand -hex 32 > beacon-data/node-key # This host's public address, published in the node's ENR (see above) PUBLIC_IP=$(curl -s https://ifconfig.me) @@ -190,7 +182,6 @@ docker run -d --name ethlambda-beacon --stop-signal SIGINT \ ghcr.io/lambdaclass/ethlambda:latest beacon \ --network mainnet \ --checkpoint-sync-url https://beaconstate.ethstaker.cc \ - --node-key /data/node-key \ --data-dir /data/db \ --http-address 0.0.0.0 \ --discovery.advertise-ip "$PUBLIC_IP" @@ -293,7 +284,6 @@ what they sign. ./target/release/ethlambda beacon \ --network mainnet \ --checkpoint-sync-url \ - --node-key ./node-key \ --data-dir ./data ``` @@ -301,7 +291,7 @@ what they sign. |---|---| | `--network` | `mainnet` (default), `sepolia`, `hoodi`, `plataberget`, or a path to a directory of network files (`config.yaml`, `genesis.ssz`, optionally `bootstrap_nodes.yaml`), the layout `eth-clients` publishes and kurtosis mounts | | `--checkpoint-sync-url` | Where the anchor comes from. Required on a fresh data directory for a built-in network, since those never start from genesis. A resumable data directory is used before it; a network loaded from a directory anchors at its own `genesis.ssz` when no URL is given | -| `--node-key` | Optional, but worth persisting: the columns this node custodies are a function of its node id, so without a key file it changes identity, and custody set, on every restart | +| `--node-key` | Optional. Without it, the node generates a key on the data directory's first start and keeps it there as `node.key`, so its identity, and with it the columns it custodies, stays the same across restarts | | `--execution-endpoint`, `--execution-jwt-secret` | Optional Engine API pairing, given together. Without them, blocks import without payload validation | A built-in network needs nothing else on disk. Its `eth-clients` `config.yaml` diff --git a/bin/ethlambda/Cargo.toml b/bin/ethlambda/Cargo.toml index 330940e2..c34dd64b 100644 --- a/bin/ethlambda/Cargo.toml +++ b/bin/ethlambda/Cargo.toml @@ -54,7 +54,8 @@ ethlambda-engine.workspace = true thiserror.workspace = true eyre.workspace = true -# For `resolve_node_key`'s ephemeral-key generation when `--node-key` is omitted. +# For `resolve_node_key`: generating the key it stores in `--data-dir` when +# `--node-key` is omitted, and checking every node key it loads. secp256k1.workspace = true diff --git a/bin/ethlambda/src/beacon.rs b/bin/ethlambda/src/beacon.rs index 7ef96a76..6cc60092 100644 --- a/bin/ethlambda/src/beacon.rs +++ b/bin/ethlambda/src/beacon.rs @@ -120,7 +120,7 @@ pub struct BeaconWireParams { pub fork_id: EnrForkId, } -/// The discv5 node id for this process's `--node-key` bytes. +/// The discv5 node id for this process's node key bytes. /// /// A thin pass-through, kept as its own function so `run_node`'s exact /// composition — resolve the key, derive the id, feed it to [`wire_params`] — @@ -141,15 +141,9 @@ pub fn beacon_node_id(node_key: &[u8]) -> eyre::Result<[u8; 32]> { /// custody set off the identity we publish. The anchor state itself belongs to /// the anchor-and-follow work, and must be checked against the genesis values /// used here when it lands. -/// -/// `node_key_supplied` carries no key material, only whether `node_id` came -/// from a persisted `--node-key` or one generated fresh for this run: this -/// function has no other way to tell the two apart, and that is exactly what -/// the startup warning below needs to know. pub fn wire_params( source: &crate::network::NetworkSource, node_id: [u8; 32], - node_key_supplied: bool, custody_group_count: u64, ) -> eyre::Result { let chain = source.config().clone(); @@ -206,9 +200,9 @@ pub fn wire_params( } // The node id is the discv5 one, so what this node custodies here is what - // any peer computes for it from its ENR. A node without a persistent - // --node-key gets a new identity and therefore a new custody set on every - // restart, which is why startup warns about it right below. + // any peer computes for it from its ENR. It stays the same across restarts + // because the node key does: `--node-key`, or the key `resolve_node_key` + // stored in the data directory beside the columns it custodies. // // `sampling_size` is the larger of the advertised count and // `SAMPLES_PER_SLOT`, so the default advertisement still custodies @@ -225,28 +219,9 @@ pub fn wire_params( "Custodying data columns" ); - // The columns above are stored under this identity and served to peers - // who compute the same set from it. An ephemeral identity makes both - // sides of that agreement stale on the next restart: the sidecars already - // on disk belong to a node id nobody, including this node, will select - // again, and the fresh id this run advertises has nothing custodied for - // it yet. Placed next to the custody log line above so an operator reads - // the two together rather than finding this warning buried in startup - // noise. - if !node_key_supplied { - warn!( - "No --node-key supplied: this node's custody columns are a function of its \ - discv5 node id, so a fresh identity on every restart means the columns already \ - stored on disk belong to a different custody set than the one this run \ - advertises and serves. Pass --node-key with a persisted key file to keep one \ - identity, and therefore one custody set, across restarts." - ); - } - - // The same node id again, and the same consequence of an ephemeral one: - // `p2p-interface.md` makes this a public function of the node id so that a - // peer can compute what this node should be listening to from its ENR - // alone. Computed at the current epoch and then kept for the process's + // The same node id again: `p2p-interface.md` makes this a public function + // of the node id so that a peer can compute what this node should be + // listening to from its ENR alone. Computed at the current epoch and then kept for the process's // lifetime; see `subnets`'s module documentation for why this does not // rotate. let attestation_subnets = @@ -352,8 +327,8 @@ mod tests { #[test] fn the_wire_parameters_are_derived_offline() { let source = crate::network::NetworkSource::built_in_mainnet().unwrap(); - let params = wire_params(&source, [0x11; 32], true, CUSTODY_REQUIREMENT) - .expect("no network is needed"); + let params = + wire_params(&source, [0x11; 32], CUSTODY_REQUIREMENT).expect("no network is needed"); assert_eq!(params.wire.genesis_time, MAINNET_GENESIS_TIME); // The digest is whatever fork the wall clock lands in, so it is not // pinned here; that it agrees with the ENR entry is the invariant. @@ -374,12 +349,12 @@ mod tests { let source = crate::network::NetworkSource::built_in_mainnet().unwrap(); - let a = wire_params(&source, [0x11; 32], true, CUSTODY_REQUIREMENT) - .expect("no network is needed"); + let a = + wire_params(&source, [0x11; 32], CUSTODY_REQUIREMENT).expect("no network is needed"); assert_eq!(a.wire.custody_columns.len(), sampling as usize); - let b = wire_params(&source, [0x22; 32], true, CUSTODY_REQUIREMENT) - .expect("no network is needed"); + let b = + wire_params(&source, [0x22; 32], CUSTODY_REQUIREMENT).expect("no network is needed"); assert_ne!(a.wire.custody_columns, b.wire.custody_columns); } @@ -391,7 +366,7 @@ mod tests { let node_id = beacon_node_id(&[0x33; 32]).expect("a well-formed key"); let source = crate::network::NetworkSource::built_in_mainnet().unwrap(); let params = - wire_params(&source, node_id, true, CUSTODY_REQUIREMENT).expect("no network is needed"); + wire_params(&source, node_id, CUSTODY_REQUIREMENT).expect("no network is needed"); let sampling = ethlambda_state_transition::beacon::das::sampling_size( ethlambda_types::beacon::constants::CUSTODY_REQUIREMENT, ); diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index 4dbe9174..eafcfbb8 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -46,10 +46,10 @@ pub(crate) struct CommonOptions { /// Hex file holding the secp256k1 key that is this node's libp2p and /// discv5 identity. /// - /// Optional on both sub-commands. When omitted, a fresh key is generated - /// in memory at startup (logged as a warning) and used for that run only, - /// so the PeerId and ENR differ on the next start. Pass a persisted key - /// file for a stable identity. + /// Optional on both sub-commands. When omitted, the key stored in + /// `--data-dir` as `node.key` is used, and on the directory's first start + /// one is generated and stored there (hex, mode `0600`), so the PeerId + /// and ENR stay the same across restarts until the directory is wiped. #[arg(long)] pub(crate) node_key: Option, /// Path to a bootnode list: ENRs, one per YAML entry. @@ -894,9 +894,10 @@ mod tests { #[test] fn an_absent_node_key_parses_on_both_sub_commands() { - // Both generate an ephemeral in-memory key instead (`main`'s - // `resolve_node_key`), so neither rejects the argv. On `node` that is - // a behavior change: `--node-key` used to be required there. + // Both use a key stored in `--data-dir` instead, generating it on + // the first start (`main`'s `resolve_node_key`), so neither rejects + // the argv. On `node` that is a behavior change: `--node-key` used to + // be required there. assert_eq!( parse_node(without(&base_args(), "--node-key")) .common diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index 6ea55328..e70dd5cd 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -370,15 +370,11 @@ async fn run_node(options: Options) -> eyre::Result<()> { return run_test_driver(rpc_config).await; } - let node_p2p_key = resolve_node_key(common.node_key.as_deref())?; - #[cfg(all(not(target_env = "msvc"), feature = "jemalloc"))] info!("Using jemalloc allocator with heap profiling enabled"); #[cfg(any(target_env = "msvc", not(feature = "jemalloc")))] info!("Using system allocator"); - info!(node_key=?common.node_key, "got node key"); - let p2p_socket = SocketAddr::new(IpAddr::from([0, 0, 0, 0]), common.gossipsub_port); // Resolved once, here, above the bootnode fallback below that reads it: @@ -440,6 +436,12 @@ async fn run_node(options: Options) -> eyre::Result<()> { .wrap_err_with(|| format!("failed to open RocksDB at {}", data_dir.display()))?, ); + // Resolved after the DB open rather than before it, because a generated + // key is stored in `data_dir`: RocksDB's lock on that directory is what + // stops two processes started on it from both generating a key and racing + // to store theirs. + let node_p2p_key = resolve_node_key(common.node_key.as_deref(), &data_dir)?; + let clean_checkpoint_urls = checkpoint_sync::clean_urls(&common.checkpoint_sync_url); // The one place the two chains diverge. Everything above is shared setup; @@ -606,12 +608,7 @@ async fn run_node(options: Options) -> eyre::Result<()> { // the same computation, so the two cannot disagree about this // node's identity. let node_id = beacon::beacon_node_id(&node_p2p_key)?; - let params = beacon::wire_params( - &source, - node_id, - common.node_key.is_some(), - mainnet.custody_group_count, - )?; + let params = beacon::wire_params(&source, node_id, mainnet.custody_group_count)?; // Cloned ahead of the move into `WireConfig::Beacon` below: the // chain actor needs its own copy to gate fulu import on, the same @@ -1312,36 +1309,76 @@ fn read_hex_file_bytes(path: impl AsRef) -> eyre::Result> { .wrap_err_with(|| format!("failed to decode hex file from {}", path.display())) } -/// Resolve a sub-command's node key: read `--node-key` if given, otherwise -/// generate a fresh secp256k1 key in memory. +/// Name of the file, inside `--data-dir`, holding the node key generated when +/// `--node-key` is absent. +const STORED_NODE_KEY_FILE: &str = "node.key"; + +/// Resolve a sub-command's node key: read `--node-key` if given, otherwise the +/// key stored in `data_dir`, generating and storing one first if the directory +/// has none yet. /// -/// Shared by both sub-commands, since `--node-key` is optional on both. There -/// is no precedent elsewhere in this binary for writing generated key material -/// to `--data-dir`, and doing so would need file permissions this repo does -/// not otherwise establish; keeping it in memory only is the conservative -/// choice, so a generated identity does not survive a restart. +/// Shared by both sub-commands, since `--node-key` is optional on both. A +/// supplied `--node-key` is only read; nothing is written. Without one, the +/// identity is kept beside the chain data it belongs to, so it survives a +/// restart: a lean node keeps its `PeerId`, and with it its place in every +/// peer's scoring and in any ENR its neighbours cached, and a `beacon` node +/// keeps its custody columns, which are a function of this same node id, so +/// the sidecars already on disk stay the ones it advertises and serves. +/// Wiping the data directory resets the identity together with that data. /// -/// The warning below is about `PeerId`/ENR churn, which costs a lean node its -/// place in every peer's scoring and in any ENR its neighbours cached. -/// `beacon` has no validator identity to protect, but it is not exempt -/// either: its custody columns are a function of this same node id, so an -/// unstable identity there means a different custody set on every restart. -/// `beacon::wire_params` carries its own, more specific warning about that; -/// see it for why `beacon` needs a second one. -fn resolve_node_key(node_key_path: Option<&Path>) -> eyre::Result> { - match node_key_path { - Some(path) => read_hex_file_bytes(path) - .wrap_err_with(|| format!("failed to load node key from {}", path.display())), - None => { - let generated = secp256k1::SecretKey::new(&mut secp256k1::rand::rngs::OsRng); - warn!( - "No --node-key supplied: generated an ephemeral secp256k1 key in memory for \ - this run only. This node's PeerId and ENR will be different on the next \ - start; pass --node-key with a persisted key file for a stable identity." - ); - Ok(generated.secret_bytes().to_vec()) - } +/// The stored file is hex, the format `--node-key` reads, so it can be passed +/// as `--node-key` to another run. It is written at mode `0600` on unix +/// through [`ethlambda_validator::secure_fs::write_private_no_symlink`], and +/// through a `.tmp` sibling renamed into place, so an interrupted first start +/// leaves no partial key behind and a symlink planted at the `.tmp` path is +/// refused rather than followed. A stored file that fails to load is an +/// error, never a cue to generate a replacement, since that would silently +/// discard an identity that was working; the error says that deleting the +/// file is how to ask for a new one. +fn resolve_node_key(node_key_path: Option<&Path>, data_dir: &Path) -> eyre::Result> { + 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); + } + + let stored = data_dir.join(STORED_NODE_KEY_FILE); + let exists = stored + .try_exists() + .wrap_err_with(|| format!("failed to look for a node key at {}", stored.display()))?; + if exists { + let key = read_node_key(&stored).wrap_err_with(|| { + format!( + "failed to load the node key stored at {}; deleting the file makes the next \ + start generate a new one, which gives this node a new identity", + stored.display() + ) + })?; + info!(path = %stored.display(), "Loaded the node key stored in the data directory"); + return Ok(key); } + + 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_no_symlink(&temporary, contents) + .and_then(|()| std::fs::rename(&temporary, &stored)) + .wrap_err_with(|| format!("failed to store a node key at {}", stored.display()))?; + info!( + path = %stored.display(), + "No --node-key supplied: generated a node key and stored it in the data directory" + ); + Ok(generated.to_vec()) +} + +/// Read a hex node key file, refusing bytes that are not a valid secp256k1 +/// secret key here rather than leaving `build_swarm` to panic on them. +fn read_node_key(path: &Path) -> eyre::Result> { + 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) } /// Fetch the initial state for the node. @@ -3260,50 +3297,127 @@ validators: } } - /// A unique path under the OS temp dir, so parallel test runs cannot - /// collide on the same file. - fn temp_key_path(label: &str) -> std::path::PathBuf { - let nanos = SystemTime::UNIX_EPOCH - .elapsed() - .expect("already past the unix epoch") - .as_nanos(); - std::env::temp_dir().join(format!("ethlambda-test-node-key-{label}-{nanos}.key")) - } - #[test] - fn a_supplied_node_key_is_read_verbatim_and_stable_across_calls() { + fn a_supplied_node_key_is_read_verbatim_and_nothing_is_stored() { // A `PeerId` is a pure function of the key bytes (see // `ethlambda_p2p::derive_peer_ids`), so two calls returning the same // bytes for the same file is exactly what "the same PeerId across two // runs" comes down to, without pulling libp2p's key derivation into // this crate's tests. - let path = temp_key_path("supplied"); + let keys = tempfile::tempdir().expect("temp dir"); + let data_dir = tempfile::tempdir().expect("temp dir"); + let path = keys.path().join("supplied.key"); std::fs::write(&path, "01".repeat(32)).expect("temp key file writes"); - let first = resolve_node_key(Some(path.as_path())).expect("reads the supplied key"); - let second = resolve_node_key(Some(path.as_path())).expect("reads the supplied key"); + let first = resolve_node_key(Some(&path), data_dir.path()).expect("reads the key"); + let second = resolve_node_key(Some(&path), data_dir.path()).expect("reads the key"); - let _ = std::fs::remove_file(&path); + assert_eq!(first, vec![0x01; 32]); assert_eq!(first, second); + let entries = std::fs::read_dir(data_dir.path()).expect("data dir reads"); + assert_eq!( + entries.count(), + 0, + "a supplied key must not be copied anywhere" + ); } #[test] - fn a_missing_node_key_generates_a_valid_key_that_differs_from_a_supplied_one() { - let path = temp_key_path("baseline"); - std::fs::write(&path, "01".repeat(32)).expect("temp key file writes"); - let supplied = resolve_node_key(Some(path.as_path())).expect("reads the supplied key"); - let _ = std::fs::remove_file(&path); - - let generated_a = resolve_node_key(None).expect("generates a key"); - let generated_b = resolve_node_key(None).expect("generates a key"); + fn a_missing_node_key_is_generated_once_and_reloaded_from_the_data_dir() { + let data_dir = tempfile::tempdir().expect("temp dir"); + let generated = resolve_node_key(None, data_dir.path()).expect("generates a key"); // "Accepted": the bytes are a valid secp256k1 secret key, the same // check `build_swarm` performs before deriving the swarm identity. - secp256k1::SecretKey::from_slice(&generated_a) + secp256k1::SecretKey::from_slice(&generated) .expect("generated key is a valid secp256k1 secret key"); - assert_ne!(generated_a, supplied); - assert_ne!(generated_a, generated_b, "two generations must not collide"); + let reloaded = resolve_node_key(None, data_dir.path()).expect("reloads the key"); + assert_eq!(generated, reloaded, "a restart must keep the identity"); + + // The stored file is in `--node-key`'s own format, so an operator can + // hand it to another run as that flag. + let stored = data_dir.path().join(STORED_NODE_KEY_FILE); + let other_data_dir = tempfile::tempdir().expect("temp dir"); + let as_flag = resolve_node_key(Some(&stored), other_data_dir.path()) + .expect("the stored file reads as --node-key"); + assert_eq!(generated, as_flag); + assert!( + !data_dir.path().join("node.key.tmp").exists(), + "the temporary sibling is renamed into place" + ); + + let elsewhere = resolve_node_key(None, other_data_dir.path()).expect("generates a key"); + assert_ne!( + generated, elsewhere, + "two data directories must not share a key" + ); + } + + #[cfg(unix)] + #[test] + fn a_generated_node_key_is_stored_at_mode_0600() { + use std::os::unix::fs::PermissionsExt as _; + + let data_dir = tempfile::tempdir().expect("temp dir"); + resolve_node_key(None, data_dir.path()).expect("generates a key"); + + let mode = std::fs::metadata(data_dir.path().join(STORED_NODE_KEY_FILE)) + .expect("the key is stored") + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o600, "got {mode:o}"); + } + + #[test] + fn a_stored_node_key_that_fails_to_load_is_an_error_not_a_new_key() { + let data_dir = tempfile::tempdir().expect("temp dir"); + let stored = data_dir.path().join(STORED_NODE_KEY_FILE); + + // Not hex, then hex of the wrong length: neither may be replaced, + // since that would silently change the node's identity. + for contents in ["not hex", "0102"] { + std::fs::write(&stored, contents).expect("writes"); + let err = resolve_node_key(None, data_dir.path()).expect_err("must refuse the file"); + assert!(err.to_string().contains(STORED_NODE_KEY_FILE), "{err:#}"); + assert!(err.to_string().contains("deleting the file"), "{err:#}"); + assert_eq!(std::fs::read_to_string(&stored).unwrap(), contents); + } + } + + #[cfg(unix)] + #[test] + fn a_symlink_at_the_temporary_node_key_path_is_not_followed() { + let data_dir = tempfile::tempdir().expect("temp dir"); + let elsewhere = tempfile::tempdir().expect("temp dir"); + let target = elsewhere.path().join("target"); + std::fs::write(&target, "untouched").expect("writes target"); + let temporary = data_dir.path().join(format!("{STORED_NODE_KEY_FILE}.tmp")); + std::os::unix::fs::symlink(&target, &temporary).expect("symlinks"); + + let err = resolve_node_key(None, data_dir.path()).expect_err("must refuse the symlink"); + let io = err + .root_cause() + .downcast_ref::() + .expect("an I/O error"); + assert_eq!(io.raw_os_error(), Some(libc::ELOOP), "{err:#}"); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "untouched"); + assert!(!data_dir.path().join(STORED_NODE_KEY_FILE).exists()); + } + + /// The key is stored in the RocksDB directory itself, after the DB is + /// opened, so RocksDB must tolerate a file it did not write there when + /// the directory is opened again. + #[test] + fn rocksdb_reopens_a_data_dir_holding_a_stored_node_key() { + let data_dir = tempfile::tempdir().expect("temp dir"); + let backend = RocksDBBackend::open(data_dir.path()).expect("opens a fresh DB"); + let generated = resolve_node_key(None, data_dir.path()).expect("generates a key"); + drop(backend); + + let _backend = RocksDBBackend::open(data_dir.path()).expect("reopens with the key present"); + let reloaded = resolve_node_key(None, data_dir.path()).expect("reloads the key"); + assert_eq!(generated, reloaded); } /// Write `contents` to a scratch file and read it back as a bootnode list. diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index ebec73b1..3dc12d12 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -43,7 +43,7 @@ pub mod payload_attestation; pub mod proposal; pub mod proposal_guard; pub mod proposer_settings; -pub(crate) mod secure_fs; +pub mod secure_fs; pub mod signing; pub mod slot_clock; pub mod subscriptions; diff --git a/crates/validator/src/secure_fs.rs b/crates/validator/src/secure_fs.rs index 574056e9..61e68c47 100644 --- a/crates/validator/src/secure_fs.rs +++ b/crates/validator/src/secure_fs.rs @@ -8,6 +8,8 @@ //! convention here is `0600`; everything in this crate that writes such a //! file must go through one of these two functions instead, to actually //! follow that convention rather than only claim to. +//! [`write_private_no_symlink`] is public for the binary's one secret of its +//! own, the node key it stores in `--data-dir` when `--node-key` is absent. use std::path::Path; @@ -50,11 +52,15 @@ pub(crate) fn write_private(path: &Path, contents: impl AsRef<[u8]>) -> std::io: /// file, so re-importing the same key (the common case: rotating its /// password, or simply retrying) overwrites its files exactly as before /// rather than failing. +/// +/// Also for the `.tmp` sibling of the binary's stored node key, which is +/// renamed into place (preserving the mode set here). Its name is fixed, and +/// the RocksDB lock the binary holds while writing it only keeps out another +/// ethlambda process, not any other local writer that could place a symlink +/// there; a leftover regular file from an interrupted start is still +/// truncated and reused. #[cfg(unix)] -pub(crate) fn write_private_no_symlink( - path: &Path, - contents: impl AsRef<[u8]>, -) -> std::io::Result<()> { +pub fn write_private_no_symlink(path: &Path, contents: impl AsRef<[u8]>) -> std::io::Result<()> { use std::io::Write as _; use std::os::unix::fs::OpenOptionsExt as _; let mut file = std::fs::OpenOptions::new() @@ -68,10 +74,7 @@ pub(crate) fn write_private_no_symlink( } #[cfg(not(unix))] -pub(crate) fn write_private_no_symlink( - path: &Path, - contents: impl AsRef<[u8]>, -) -> std::io::Result<()> { +pub fn write_private_no_symlink(path: &Path, contents: impl AsRef<[u8]>) -> std::io::Result<()> { std::fs::write(path, contents) } diff --git a/docs/architecture.md b/docs/architecture.md index 9a4df60e..99b6f7fd 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -246,10 +246,12 @@ spawn is fail-fast, so a misconfigured node stops at boot instead of hours later 1. Install the tracing subscriber, parse CLI options, register metrics, raise the file-descriptor limit for RocksDB's unbounded table cache. -2. Load the node key, then genesis config, validator config, bootnode ENRs and validator keys. -3. Open the database, then pick an anchor: resume from disk if the on-disk head is recent - enough, otherwise [checkpoint sync](./checkpoint_sync.md) from the configured URLs, and - otherwise build the genesis state. A stale database with no checkpoint URL configured is +2. Load genesis config, validator config, bootnode ENRs and validator keys. +3. Open the database, then load the node key: `--node-key`, else the `node.key` stored in + the data directory, generated there on its first start. Then pick an anchor: resume from + disk if the on-disk head is recent enough, otherwise + [checkpoint sync](./checkpoint_sync.md) from the configured URLs, and otherwise build the + genesis state. A stale database with no checkpoint URL configured is resumed anyway, with a warning, since that is the setup the node was given. 4. Build the shared handles: aggregator controller, sync-status controller, event bus, and the subnet set that both the swarm and the actor need to agree on. diff --git a/docs/cli.md b/docs/cli.md index 2dcbf670..6d1a1b4d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -57,7 +57,7 @@ one `CommonOptions` struct flattened into each. | `--http-address` | `127.0.0.1` | Bind address for both HTTP servers | | `--api-port` | `5052` | API server port. `beacon` binds it too, off its own anchored store rather than an empty one; the lean-shaped `/lean/v0` routes still don't answer for it (see [`beacon` flags](#beacon-flags) below) | | `--metrics-port` | `5054` | Metrics and debug server port. Equal to `--api-port` merges the routers onto one listener | -| `--node-key` | generates an ephemeral key | Hex file holding the secp256k1 key that is this node's libp2p and discv5 identity. When omitted, a fresh key is generated in memory each start (logged as a warning), so the PeerId and ENR differ on every restart | +| `--node-key` | `/node.key` | Hex file holding the secp256k1 key that is this node's libp2p and discv5 identity. When omitted, the key stored in `--data-dir` as `node.key` is used, generated and stored there (mode `0600`) on the directory's first start, so the PeerId and ENR stay the same across restarts until the directory is wiped | | `--bootnodes` | see below | Bootnode ENR list: one `enr:...` per line, as a YAML block sequence, a YAML flow sequence, or a plain list (`#` comments and leading `- ` are both tolerated) | | `--checkpoint-sync-url` | see below | API base URLs, tried in order until one answers. Supplies `node`'s starting state on the lean API, and `beacon`'s anchor on a standard Beacon API; required on a fresh `beacon` data directory only for a built-in network, since a loaded network anchors at its own `genesis.ssz` instead. See [`checkpoint_sync.md`](checkpoint_sync.md) | | `--discovery.port` | `9000` | discv5 UDP port; must differ from `--gossipsub-port`. See [Peer discovery](./discovery.md) | @@ -70,7 +70,7 @@ carries none and each chain resolves an absent value itself: | Flag | Absent on `node` | Absent on `beacon` | |---|---|---| -| `--node-key` | ephemeral in-memory key, warned about | same | +| `--node-key` | `/node.key`, generated on the directory's first start | same | | `--bootnodes` | no bootnodes: peers only via discv5 (with `--discovery.enable`) or by being dialed, warned about | falls back to the resolved network's own bootnode list: a built-in network's embedded list, or a loaded directory's `bootstrap_nodes.yaml`/`.txt` | | `--checkpoint-sync-url` | start from a resumable DB, else from genesis | start from a resumable DB, else anchor at the resolved network's own genesis (loaded network only), else abort | @@ -230,11 +230,11 @@ not chain-specific, branches once, and shares the shutdown: | register metrics, print the banner, log the version | shared | shared | | raise `RLIMIT_NOFILE` | shared | shared | | `HIVE_LEAN_TEST_DRIVER` early return | yes | no: those endpoints are lean's | -| resolve `--node-key` | shared | shared | | resolve `--network` into a `NetworkSource` | n/a: lean has no `--network` | classify the value (built-in name vs. directory); parse the `config.yaml` (embedded, or the directory's) and check `PRESET_BASE` against the compiled preset and the constant-backed keys against their constants; for a directory, then decode `genesis.ssz` | | read `--bootnodes` (falls back to the chain's default list; see the table above) | shared | shared | | build the aggregator, sync-status and event handles | shared | shared | | open `--data-dir`'s RocksDB backend | shared | shared | +| resolve `--node-key`, else load `/node.key`, generating it on first start (after the open, so RocksDB's lock covers the write) | shared | shared | | **the one `match`**: produce a `ChainSetup` | genesis config, validator keys, checkpoint sync or resume onto the shared backend, subnets | `beacon::wire_params` (genesis metadata, epoch, fork digest), then checkpoint sync or resume onto the same backend | | build the swarm, spawn P2P, start discv5 | shared; discv5 only with `--discovery.enable` | shared | | start the HTTP server | shared | shared | diff --git a/docs/data_storage.md b/docs/data_storage.md index a88050a8..ac83e8ee 100644 --- a/docs/data_storage.md +++ b/docs/data_storage.md @@ -918,6 +918,15 @@ twelve-second ones. A staler DB, or one whose anchor checkpoint state is missing, falls through to checkpoint sync, which writes a fresh anchor on top of the existing data. +One file in `--data-dir` is not RocksDB's: `node.key`, the node's secp256k1 +libp2p and discv5 identity. When `--node-key` is absent, `run_node` reads it on +every start and generates it on the first (hex, mode `0600`). It never goes +through the `StorageBackend`, is written only after the DB is open so RocksDB's +lock covers it, and is lost with the directory when the directory is wiped, +together with the custody columns that identity selected. A `node.key` that +fails to load stops startup rather than being replaced; deleting it is how to +ask for a new identity. + ## Key Files | File | Component |