feat(beacon): report the node's ENR from /node/identity, and discover keystores without a definitions file

[pablodeymo]

Moved from lambdaclass/ethlambda_private#57. Open review there: store the ENR in the Store and serve it from there; agreed to add the plumbing for updating it now, leaving the actual updating for later. Not yet addressed.

🗒️ Description / Motivation

Three small changes that let an ethereum-package devnet use ethlambda as a real participant: as a peer or bootnode for other consensus clients, and as a validator client fed ethereum-package's own keystore artifact. They were written for lambdaclass/ethlambda_private#16 but pushed after it merged; they are rebased onto beacon-chain-integration here, now that lambdaclass/ethlambda_private#34 and lambdaclass/ethlambda_private#40 are merged.

1. /eth/v1/node/identity reported empty enr and p2p_addresses. ethereum-package reads .data.enr and .data.p2p_addresses[0] from every consensus client to wire bootnodes, so ethlambda beacon could not serve as a peer or a bootnode. This was recorded as a spec deviation.
2. The padded ENR broke Lighthouse. Once (1) reported the ENR, Lighthouse refused it ("Not valid as ENR nor Multiaddr") and exited before opening its HTTP port. EIP-778's text form is URL-safe base64 without padding, and ethrex's enr_url pads it.
3. ethlambda validator required a hand-written validator_definitions.yml. The layout eth2-val-tools, staking-deposit-cli and ethereum-package produce has no such file, so an ethereum-package vc_type for ethlambda could not pass the keystore artifact straight through, as its Lighthouse launcher does.

What Changed

• 48ebe3c: report the ENR and listen addresses
  • P2P::local_enr keeps the ENR discovery published at startup.
  • main.rs builds a BeaconIdentity from it: the QUIC and TCP libp2p multiaddrs and the discv5 one, from --discovery.advertise-ip and the two ports.
  • It reaches the handler through BeaconApiHandles.
  • Removes the entry from docs/spec_deviations.md and updates docs/rpc.md.
• 1b1a985: discover keystores without a definitions file (crates/validator/src/keys/definitions.rs, bin/ethlambda/src/validator.rs, docs/cli.md)
  • discover_if_absent scans <validators-dir>/<0xpubkey>/voting-keystore.json and pairs each with <secrets-dir>/<0xpubkey>.
  • It writes the definitions file, sorted so it is reproducible, before the store loads it.
• 6a2915d: publish the ENR without base64 padding (crates/net/p2p/src/discovery/mod.rs).

Correctness / Behavior Guarantees

• No guessed addresses. Without --discovery.advertise-ip, p2p_addresses and discovery_addresses stay empty: a node bound to 0.0.0.0 doesn't know the address peers reach it on.
• The handler still answers when no identity is layered, with empty network fields.
• An existing definitions file is never rescanned. It is the keymanager's record, and a rescan would bring back a key it deleted.
• Keys are still checked. The store still checks every declared key against its decrypted secret, as before.
• Only padding changes in the ENR. The record's content and signature are unchanged; a padded record decodes to the same bytes.

Tests Added / Run

• Padding: the existing discovery test now also asserts the published record carries no padding.
• Keystore discovery:
  • keystores_in_the_lighthouse_layout_are_discovered_when_no_file_exists
  • an_existing_definitions_file_is_never_rescanned
• Identity:
  • identity_reports_the_enr_and_listen_addresses
  • identity_without_an_identity_carries_the_peer_id_and_empty_network_fields
• Rebase: one conflict, in main.rs, around the RPC handles. The resolution keeps this PR's rpc_identity binding and drops the duplicate SharedAttestationPool lines the original commit carried as context, since lambdaclass/ethlambda_private#40 already removed that duplicate.
• Devnet: the ethereum-package runs described in the commit messages predate the rebase and have not been repeated on this base.

Related Issues / PRs

• Builds on lambdaclass/ethlambda_private#34 and lambdaclass/ethlambda_private#40, both now merged.
• Originally part of the lambdaclass/ethlambda_private#16 work (merged without these three).

✅ 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 (1845 passed, 0 failed, 25 ignored)

[github-actions]

🤖 Claude Code Review

Review of PR 644: ENR and multiaddrs in /eth/v1/node/identity, Lighthouse-layout keystore discovery

The change is small and reads well. I found no consensus-layer or memory-safety problems. The findings below are minor.

Findings

1. Misplaced doc comment (crates/validator/src/keys/definitions.rs, around the new discover_if_absent)

   • discover_if_absent was inserted between the /// The entries that should be loaded and signed with. line and pub fn enabled.
   • Rustdoc now attaches that line to discover_if_absent as its first paragraph, and enabled has no doc.
   • Move discover_if_absent above that comment, or put it after enabled.

2. Discovery is stricter than Lighthouse's, and one bad keystore aborts startup (discover_if_absent)

   • Keystore::from_json(..).ok() discards the parse error. A malformed keystore and a keystore with no pubkey both produce "must name its public key", which hides the real cause.
   • The ? on that error fails the whole run, so one stray voting-keystore.json in the directory blocks every validator. Consider warn! and continue, or at least keep the underlying error in the message.
   • The pubkey field is also unvalidated. It is joined into secrets_dir.join(&pubkey), so a keystore carrying ../ in pubkey would build a password path outside secrets_dir. The validators directory is operator-controlled, so this is low severity. A hex-length check on the pubkey would close it cheaply.
   • Lowercasing the pubkey would also avoid mismatches with password filenames.

3. Silent failure on an unreadable directory entry (discover_if_absent)

   • entry.path().join("voting-keystore.json").is_file() returns false on permission errors, so an unreadable keystore directory is skipped without a log line. Debug-level logging per skipped directory would help operators.

4. ENR can go stale (crates/net/p2p/src/lib.rs, local_enr)

   • The new doc comment already notes that discv5 may re-sign the record with a higher seq after IP voting. The endpoint then serves the startup record forever.
   • That is acceptable for a bootnode-wiring use case. metadata.seq_number still reports "0", which is inconsistent with an ENR whose seq is not 0. Worth a note in docs/spec_deviations.md, since the PR removes the old entry entirely.

5. ENR padding trim (crates/net/p2p/src/discovery/mod.rs)

   • trim_end_matches('=') is correct for EIP-778 and the test covers it. The decode_enr helper in the tests must accept unpadded input. It does, since the tests pass.
   • Only the string handed out is trimmed, so it is consistent.
   • The trim assumes enr_url only ever pads at the end, which holds for base64.

6. Multiaddr formatting (bin/ethlambda/src/main.rs, beacon_identity)

   • ip.to_string() for IPv6 gives a valid multiaddr component, so this is fine.
   • The discv5 address /udp/<port>/p2p/<id> is non-standard but harmless.
   • With advertise_ip unset, the ENR is still returned but carries no IP, as documented.
   • If --gossipsub-port is 0, the multiaddrs will report port 0. The CLAUDE.md notes say ports are validated, but discovery.port can be 0 in tests, which may leave a wrong value here. Consider skipping the addresses when a port is 0, to match the "0 means absent" convention.

7. API shape (crates/net/rpc/src/beacon/node.rs)

   • Taking Option<Extension<BeaconIdentity>> per request clones the strings each time. That is cheap, and fine.
   • Using an Extension rather than state matches the existing pattern for p2p, engine and the other handles.
   • Making identity a required field of BeaconApiHandles while the handler tolerates its absence is a little redundant, but it is useful for tests.

Tests and docs

• The tests cover the layout, ordering, the don't-rescan case and the ENR padding.
• Add a test for a malformed or missing-pubkey keystore, to pin whichever behavior you choose in Item 2.
• The doc updates in docs/cli.md, docs/rpc.md, the validator.rs flag help and the spec_deviations.md removal are consistent with each other.

Verdict: Fix Item 1 before merging. Item 2 (error handling and pubkey validation) is worth addressing. The rest are optional.

---

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

[github-actions]

🤖 Codex Code Review

I found 2 review items.

1. bin/ethlambda/src/main.rs:127 / bin/ethlambda/src/main.rs:138
beacon_identity() suppresses both address lists when --discovery.advertise-ip is unset, but it still returns the ENR. That ENR may already contain dialable addressing once discv5 IP voting updates it, as your own discovery path suggests in crates/net/p2p/src/discovery/mod.rs:257. This creates an inconsistency in /eth/v1/node/identity: enr can advertise reachability while p2p_addresses and discovery_addresses stay empty forever because they are snapshotted once at startup from CLI config.
   Suggestion: either derive the address lists from the current ENR/listen addresses at request time, or clearly document/encode that these fields are startup-only and may diverge from the published ENR.

2. bin/ethlambda/src/main.rs:135
   The new QUIC multiaddr is emitted as /<family>/<ip>/udp/<gossipsub_port>/quic-v1/p2p/<peer_id>. libp2p multiaddrs usually place /p2p/<peer_id> before transport upgrades only when describing nested circuits; for direct listener addresses the common form is /ip4/.../udp/.../quic-v1 with the peer ID appended last only if needed by the caller. Since this endpoint is intended for external consumers, it would be worth adding a test that parses these strings back into Multiaddr and confirms they are accepted by the same stack ethlambda dials with. Without that, there is some interoperability risk around the exact address shape.

Otherwise this looks clean:

• trimming = from the ENR in crates/net/p2p/src/discovery/mod.rs:194 matches EIP-778 text encoding and the added regression test is good;
• the RPC wiring via Extension(handles.identity) is straightforward and low-risk;
• no obvious consensus, attestation, fork-choice, SSZ, or signature-path correctness changes are introduced by this PR.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the areas you specified. This PR adds Beacon API /eth/v1/node/identity support and Lighthouse-compatible keystore discovery.

Critical Issues

1. ENR Padding Fix - Potential Panic on Empty String (crates/net/p2p/src/discovery/mod.rs:198-200)

let local_enr = local_record
    .enr_url()
    .map_err(DiscoveryError::EncodeEnr)?
    .trim_end_matches('=')
    .to_string();

Bug: If enr_url() returns "enr:" (possible edge case), trimming = from the end is harmless, but if it returns just "enr" somehow, the result is still valid. However, the real issue: trim_end_matches removes all trailing = characters. EIP-778 base64url encoding without padding should have no = at all, but this is correct behavior.

More serious: The local_enr is now stored in P2P struct and returned via local_enr(). But the ENR can be re-signed by discv5 with a higher sequence number when IP voting changes the external address. The comment in crates/net/p2p/src/lib.rs:1112-1114 acknowledges this, but consumers of /eth/v1/node/identity may get a stale ENR that doesn't match the currently broadcast one.

Suggestion: Consider whether the identity endpoint should reflect the current ENR rather than the startup snapshot, or document this limitation more prominently.

2. IPv6 Address Formatting Bug (bin/ethlambda/src/main.rs:129-132)

let family = if ip.is_ipv4() { "ip4" } else { "ip6" };
// ...
format!("/{family}/{ip}/udp/{gossipsub_port}/quic-v1/p2p/{peer_id}"),

Bug: IPv6 addresses in multiaddr format must be enclosed in square brackets when used with /ip6/. The Display impl for IpAddr will print an IPv6 address like 2001:db8::1, but multiaddr format requires /ip6/2001:db8::1 — actually, checking the multiaddr spec, /ip6/ uses the textual format without brackets. This appears correct per spec. Disregard.

Wait — re-checking: The Display impl for Ipv6Addr prints without brackets (2001:db8::1), and /ip6/2001:db8::1 is valid multiaddr format. This is correct.

Security Issues

3. Path Traversal in Keystore Discovery (crates/validator/src/keys/definitions.rs:102-120)

pub fn discover_if_absent(validators_dir: &Path, secrets_dir: &Path) -> Result<usize> {
    // ...
    let keystore_path = entry.path().join("voting-keystore.json");
    // ...
    let pubkey = format!("0x{}", pubkey.trim_start_matches("0x"));
    definitions.push(ValidatorDefinition {
        enabled: true,
        voting_keystore_password_path: secrets_dir.join(&pubkey),
        // ...
    });

Vulnerability: The pubkey is read from the keystore JSON and used directly as a path component. While EIP-2335 pubkeys are hex strings that should be valid filenames, there's no validation that the pubkey field contains only hex characters. A malicious keystore could contain ../../../etc/passwd as its "pubkey", leading to:

1. The password path escaping secrets_dir
2. Potential information disclosure if the file exists and is read later

Fix (line 113-114):

let pubkey = crate::keys::keystore::Keystore::from_json(&json)
    .ok()
    .and_then(|keystore| keystore.pubkey)
    .ok_or_else(|| Error::Keystore { ... })?;
// Add validation:
if !pubkey.chars().all(|c| c.is_ascii_hexdigit()) {
    return Err(Error::Keystore {
        path: keystore_path.display().to_string(),
        reason: "pubkey contains non-hex characters".to_string(),
    });
}
let pubkey = format!("0x{}", pubkey.trim_start_matches("0x"));

4. TOCTOU Race Condition in File Existence Check (crates/validator/src/keys/definitions.rs:97-100)

pub fn discover_if_absent(validators_dir: &Path, secrets_dir: &Path) -> Result<usize> {
    if validators_dir.join(DEFINITIONS_FILE).exists() {
        return Ok(0);
    }

Classic time-of-check to time-of-use race. Between the exists() check and subsequent operations, the file could be created or removed. This is low severity for validator startup (not attacker-exploitable in a meaningful way), but unidiomatic.

Fix: Use std::fs::OpenOptions::read(true).open() and handle AlreadyExists or similar, or simply accept that save() will fail if the file was created concurrently. The current behavior is acceptable for this use case.

Correctness & Logic Issues

5. Missing discovery_addresses Assertion in Test (crates/net/rpc/src/beacon/node.rs:185-195)

#[tokio::test]
async fn identity_without_an_identity_carries_the_peer_id_and_empty_network_fields() {
    let json = body_json(get("/eth/v1/node/identity").await).await;
    assert_eq!(json["data"]["peer_id"], "test-peer");
    assert_eq!(json["data"]["enr"], "");
    assert_eq!(json["data"]["p2p_addresses"], serde_json::json!([]));
    assert_eq!(json["data"]["metadata"]["seq_number"], "0");
}

Bug: The test name says "empty network fields" but discovery_addresses assertion is missing. This was present in the original test (line 175 in the diff shows removal). The new test is incomplete.

Fix (add after line 193):

    assert_eq!(json["data"]["discovery_addresses"], serde_json::json!([]));

6. Incorrect Port Used for Gossipsub QUIC/TCP (bin/ethlambda/src/main.rs:131-132)

p2p_addresses: vec![
    format!("/{family}/{ip}/udp/{gossipsub_port}/quic-v1/p2p/{peer_id}"),
    format!("/{family}/{ip}/tcp/{gossipsub_port}/p2p/{peer_id}"),
],

Question: Is gossipsub_port correct for both? The comment says "QUIC first, matching the order this node dials in." Gossipsub typically runs over libp2p, which may use a different port than the QUIC transport. Verify that gossipsub_port is indeed the libp2p listen port and not just the gossipsub topic port. If the swarm is configured to listen on a different port, this will advertise incorrect addresses.

This may be correct by convention, but worth double-checking against SwarmConfig and how build_swarm sets up listeners.

Performance & Design

7. Unnecessary String Cloning (crates/net/p2p/src/lib.rs:1042)

let local_enr = discovery.local_enr.clone();

The local_enr is already a String. discovery is consumed by this point (it's used for spawn_listener later), but actually spawn_discovery returns a Discovery handle, not the record. Check if Discovery needs to retain ownership. If DiscoveryHandle is returned, cloning may be necessary. Looking at the code, discovery is used later:

let disc_tx = discovery.disc_tx.clone();
// ...
spawn_listener(handle.context(), discovery.disc_rx.map(WrappedDiscEvent));

So discovery is not fully consumed. The clone is likely necessary. Acceptable.

8. Extension Extraction Could Be Cleaner (crates/net/rpc/src/beacon/node.rs:20-24)

get(move |identity: Option<Extension<crate::BeaconIdentity>>| {
    get_identity(
        peer_id.clone(),
        identity.map(|Extension(identity)| identity),
    )
}),

This works but the Option<Extension<T>> pattern with map(|Extension(identity)| identity) is awkward. In axum, Extension<T> extractor fails with 500 if not present, while Option<Extension<T>> gives None. This is correct for backward compatibility.

However, consider using Extension<Option<BeaconIdentity>> or restructuring the layer setup. The current approach is fine.

Rust Idioms & Style

9. Missing use for IpAddr (bin/ethlambda/src/main.rs)

The function beacon_identity takes Option<IpAddr> but there's no visible use std::net::IpAddr; in the diff. Verify this is imported at module level or via prelude.

10. Doc Comment Placement (crates/validator/src/keys/definitions.rs:79-80)

    /// The entries that should be loaded and signed with.
+    /// Write a definitions file for a validators directory that has none...

The doc comment for enabled() now has discover_if_absent()'s documentation inserted before it. The /// The entries that should be loaded and signed with. comment belongs to enabled(), but the new function's docs are placed between them. This is confusing.

Fix: Move the new doc comment to be directly on discover_if_absent, and restore enabled()'s doc comment placement. Currently:

    /// The entries that should be loaded and signed with.  <-- This doc is orphaned!
    /// Write a definitions file...  <-- This doc applies to discover_if_absent below
    pub fn discover_if_absent(...) -> Result<usize> {

Should be:

    /// Write a definitions file...
    pub fn discover_if_absent(...) -> Result<usize> {
    }

    /// The entries that should be loaded and signed with.
    pub fn enabled(&self) -> ...

Minor Issues

11. Test Helper Name Clarity (crates/validator/src/keys/definitions.rs:151)

fn write_keystore(dir: &Path, pubkey: &str) {

This writes a keystore with the given pubkey, but the function name doesn't indicate it's also creating the <0xpubkey>/ subdirectory. Acceptable.

12. Comment Typo (bin/ethlambda/src/main.rs:115)

/// publishes, and the multiaddrs peers reach this node on.

Should be "peers can reach this node on" or "peers use to reach this node". Minor grammar.

Consensus-Critical Assessment

This PR does not touch consensus-critical code:

• No fork choice modifications
• No attestation processing changes
• No justification/finalization logic
• No state transition functions
• No XMSS signature verification
• No SSZ encoding/decoding

The changes are purely in:

• RPC/node identity reporting
• Validator client keystore discovery
• P2P ENR formatting

These are node operational concerns, not consensus logic. The ENR padding fix is interoperability-critical but not consensus-critical.

Summary

Priority	Issue	Location
Critical	Missing discovery_addresses assertion in test	crates/net/rpc/src/beacon/node.rs:193
Security	Path traversal via unvalidated pubkey	crates/validator/src/keys/definitions.rs:113-114
Medium	Stale ENR after discv5 re-signs	crates/net/p2p/src/lib.rs:1112 (design limitation)
Low	Doc comment placement	crates/validator/src/keys/definitions.rs:79-80
Low	TOCTOU race in file check	crates/validator/src/keys/definitions.rs:98

The PR is generally well-structured and the ENR padding fix is correct. The main concerns are the incomplete test and the potential path traversal in keystore discovery.

---

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