From fd305e83de0ea60de28e92f846a0e62c8fac0824 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:21:20 -0300 Subject: [PATCH 1/3] feat(discovery): republish the ENR's eth2 entry at fork boundaries The served ENR kept its startup digest across every fork-digest boundary, because ethrex gave a consumer no way to edit the record it serves. Peers discovering a long-running node after a boundary read the stale digest and rejected it. ethrex's feat/discv5-lock-free-local-enr makes the SharedLocalNode slot the only copy of the served record: an ArcSwap that discovery reads on every packet and that every writer updates by compare-and-swap. It also adds edit_record, which re-signs with one seq bump and re-applies the edit if IP voting swapped a record in first. The pin moves there, and arc-swap enters the lockfile at 1.9.2, since 1.9.3 is still inside the publish-age cooldown. LocalEnrWriter holds the slot and the node key, and transition::apply offers it the schedule's ENRForkID on every call. It edits only when the served entry differs, since every edit bumps seq and a bumped seq sends each peer back for the whole record. It is a separate, crate-private type so the key does not ride along with LocalEnr into the RPC server. LocalEnr itself now reads the slot without a lock. --- Cargo.lock | 44 +++-- crates/net/p2p/Cargo.toml | 23 +-- crates/net/p2p/src/beacon/transition.rs | 55 +++++- crates/net/p2p/src/discovery/dial.rs | 11 +- crates/net/p2p/src/discovery/mod.rs | 215 +++++++++++++++++------- 5 files changed, 252 insertions(+), 96 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c1f6c1267..498aab0cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -192,6 +192,15 @@ version = "1.0.102" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" +[[package]] +name = "arc-swap" +version = "1.9.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c049c0be4daef0b145cb3555416b3b8ef5b7888a38aea1a3a155801fe7b0810b" +dependencies = [ + "rustversion", +] + [[package]] name = "ark-bn254" version = "0.5.0" @@ -1870,7 +1879,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2254,7 +2263,7 @@ dependencies = [ [[package]] name = "ethrex-blockchain" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "crossbeam", @@ -2279,7 +2288,7 @@ dependencies = [ [[package]] name = "ethrex-common" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "crc32fast", @@ -2314,7 +2323,7 @@ dependencies = [ [[package]] name = "ethrex-crypto" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "ark-bn254", "ark-ec", @@ -2336,7 +2345,7 @@ dependencies = [ [[package]] name = "ethrex-guest-program" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "ethereum-types", @@ -2359,7 +2368,7 @@ dependencies = [ [[package]] name = "ethrex-l2-common" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "ethereum-types", @@ -2377,7 +2386,7 @@ dependencies = [ [[package]] name = "ethrex-levm" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "derive_more 1.0.0", @@ -2395,7 +2404,7 @@ dependencies = [ [[package]] name = "ethrex-metrics" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "ethrex-common", "serde", @@ -2407,10 +2416,11 @@ dependencies = [ [[package]] name = "ethrex-p2p" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "aes", "aes-gcm", + "arc-swap", "bytes", "concat-kdf", "crossbeam", @@ -2449,7 +2459,7 @@ dependencies = [ [[package]] name = "ethrex-rlp" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "ethereum-types", @@ -2459,7 +2469,7 @@ dependencies = [ [[package]] name = "ethrex-storage" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "anyhow", "bytes", @@ -2482,7 +2492,7 @@ dependencies = [ [[package]] name = "ethrex-trie" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "anyhow", "bytes", @@ -2502,7 +2512,7 @@ dependencies = [ [[package]] name = "ethrex-vm" version = "30.0.1" -source = "git+https://github.com/lambdaclass/ethrex?rev=8f84752408a126cfa8a9c21c3ebaa76658023cb7#8f84752408a126cfa8a9c21c3ebaa76658023cb7" +source = "git+https://github.com/lambdaclass/ethrex?rev=80308d0df591f97a3bb1f81cd868f13b9729b48e#80308d0df591f97a3bb1f81cd868f13b9729b48e" dependencies = [ "bytes", "derive_more 1.0.0", @@ -3284,7 +3294,7 @@ dependencies = [ "js-sys", "log", "wasm-bindgen", - "windows-core 0.62.2", + "windows-core 0.57.0", ] [[package]] @@ -6398,7 +6408,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -7152,7 +7162,7 @@ dependencies = [ "getrandom 0.4.2", "once_cell", "rustix", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -7964,7 +7974,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] diff --git a/crates/net/p2p/Cargo.toml b/crates/net/p2p/Cargo.toml index cecda325d..7f604c06c 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -51,20 +51,23 @@ rand = "0.8" # Required for NodeEnr parsing. # -# Pinned to the head of ethrex's `feat/discv5-predicate-queries`, which builds -# on PR #7217. #7217 moves the contact table into the discovery server and gives -# a consumer `DiscoveryHandle::record_peer_event` to report its own connections -# with, which is what lets a libp2p client feed ethrex's lookup pacing. The -# branch adds `DiscoveryHandle::find_nodes`/`records_matching`, the predicate -# queries the subnet hunt in `discovery/subnets.rs` runs on. See -# `docs/discovery.md`. +# Pinned to the head of ethrex's `feat/discv5-lock-free-local-enr`, which builds +# on `feat/discv5-predicate-queries`, itself on PR #7217. #7217 moves the contact +# table into the discovery server and gives a consumer +# `DiscoveryHandle::record_peer_event` to report its own connections with, which +# is what lets a libp2p client feed ethrex's lookup pacing. The middle branch +# adds `DiscoveryHandle::find_nodes`/`records_matching`, the predicate queries +# the subnet hunt in `discovery/subnets.rs` runs on. The top one makes the +# `SharedLocalNode` slot the only copy of the served record and lets a consumer +# edit it (`edit_record`), which is how a fork boundary moves our `eth2` entry. +# See `docs/discovery.md`. # # A commit rather than a branch, for the reason the release tag it replaces was # chosen over `main`: what this crate builds against must not move without the # manifest saying so. Goes back to a tag once both are in a release. -ethrex-p2p = { git = "https://github.com/lambdaclass/ethrex", rev = "8f84752408a126cfa8a9c21c3ebaa76658023cb7" } -ethrex-rlp = { git = "https://github.com/lambdaclass/ethrex", rev = "8f84752408a126cfa8a9c21c3ebaa76658023cb7" } -ethrex-common = { git = "https://github.com/lambdaclass/ethrex", rev = "8f84752408a126cfa8a9c21c3ebaa76658023cb7" } +ethrex-p2p = { git = "https://github.com/lambdaclass/ethrex", rev = "80308d0df591f97a3bb1f81cd868f13b9729b48e" } +ethrex-rlp = { git = "https://github.com/lambdaclass/ethrex", rev = "80308d0df591f97a3bb1f81cd868f13b9729b48e" } +ethrex-common = { git = "https://github.com/lambdaclass/ethrex", rev = "80308d0df591f97a3bb1f81cd868f13b9729b48e" } # `rand` seeds the throwaway signing keys the ENR tests build records with. secp256k1.workspace = true diff --git a/crates/net/p2p/src/beacon/transition.rs b/crates/net/p2p/src/beacon/transition.rs index 67cdf18cb..fbba56197 100644 --- a/crates/net/p2p/src/beacon/transition.rs +++ b/crates/net/p2p/src/beacon/transition.rs @@ -22,7 +22,7 @@ use tracing::{error, info}; use super::BeaconWire; use super::topics::{self, BeaconTopics}; -use crate::{P2PServer, Wire, discovery, metrics}; +use crate::{P2PServer, Wire, metrics}; /// How far before a boundary a peer may already be on the next digest and still /// count as current: clocks disagree by up to the gossip clock disparity. @@ -114,9 +114,9 @@ pub(crate) fn advance(server: &mut P2PServer) -> Option { /// aggregator-subnet topics already held for the current slot. /// 3. Leave the topics of every digest the window no longer covers. /// 4. If the current digest changed, switch what this node publishes and -/// advertises: digest, fork, metadata sequence number, metric, and the -/// `eth2` entry. -/// 5. Point the admission filter at the new entry, admitting every held digest. +/// advertises: digest, fork, metadata sequence number and metric. +/// 5. Point the admission filter at the `eth2` entry the schedule prescribes at +/// `epoch`, admitting every held digest, and serve that entry in the ENR. pub(crate) fn apply(server: &mut P2PServer, epoch: Epoch) -> Changes { let mut changes = Changes::default(); let Wire::Beacon(wire) = &mut server.wire else { @@ -244,12 +244,13 @@ pub(crate) fn apply(server: &mut P2PServer, epoch: Epoch) -> Changes { .iter() .map(|topics| topics.fork_digest) .collect(); - // Without discovery there is no admission policy to update. + // Without discovery there is no admission policy to update and no ENR to + // serve. The ENR is offered the entry on every call, not only on a switch: + // it skips one it already carries, so this costs nothing between + // boundaries and keeps the served record on the schedule's answer. if let Some(state) = &server.discovery { state.filter().set_fork_id(fork_id, also_admitted); - } - if switched { - discovery::update_served_enr(&fork_id); + state.local_enr_writer().publish_fork_id(&fork_id); } changes } @@ -265,6 +266,7 @@ fn sync_committee_topic(digest: ForkDigest, subnet_id: u64) -> IdentTopic { #[cfg(test)] mod tests { use super::*; + use crate::discovery::enr::{ETH2_ENR_KEY, EnrForkId}; use crate::test_support::unconnected_beacon_server; use ethlambda_types::beacon::fork::ForkName; use libssz_types::SszList; @@ -379,6 +381,43 @@ mod tests { assert_eq!(server.wire.beacon().unwrap().fork, ForkName::Gloas); } + /// The `eth2` entry discovery serves, and the `seq` it is served under. + fn served_eth2(server: &P2PServer) -> (EnrForkId, u64) { + use libssz::SszDecode; + let record = server + .discovery + .as_ref() + .expect("the test server runs discovery") + .local_enr_writer() + .served_record(); + let raw = record.pairs().extra(ETH2_ENR_KEY).expect("eth2 entry"); + let fork_id = EnrForkId::from_ssz_bytes(&raw).expect("a valid ENRForkID"); + (fork_id, record.seq) + } + + #[tokio::test] + async fn a_crossing_moves_the_served_eth2_entry_once() { + let mut server = unconnected_beacon_server(config(), 0).await; + let schedule = server.wire.beacon().expect("beacon").schedule.clone(); + + apply(&mut server, BPO - 1); + let (served, seq) = served_eth2(&server); + assert_eq!(served, schedule.enr_fork_id(BPO - 1)); + + // Between boundaries the entry is already right, so the record is left + // alone: a bumped `seq` would send peers back for an identical record. + apply(&mut server, BPO - 1); + assert_eq!(served_eth2(&server), (served, seq)); + + // At the boundary peers discovering the node read the new digest, under + // one new `seq`. + apply(&mut server, BPO); + let (crossed, crossed_seq) = served_eth2(&server); + assert_eq!(crossed, schedule.enr_fork_id(BPO)); + assert_eq!(crossed.fork_digest, schedule.digest_at(BPO)); + assert_eq!(crossed_seq, seq + 1); + } + #[tokio::test] async fn a_node_started_inside_the_window_joins_both_digests() { let config = config(); diff --git a/crates/net/p2p/src/discovery/dial.rs b/crates/net/p2p/src/discovery/dial.rs index 93c3131eb..bfcc46009 100644 --- a/crates/net/p2p/src/discovery/dial.rs +++ b/crates/net/p2p/src/discovery/dial.rs @@ -26,7 +26,7 @@ use super::node_id::node_id_from_peer_id; use super::subnets::SubnetHunt; use super::{ DIAL_INTERVAL_AT_TARGET, DIAL_INTERVAL_AT_ZERO_PEERS, DISCOVERY_CANDIDATE_BATCH, - SpawnedDiscovery, + LocalEnrWriter, SpawnedDiscovery, }; use crate::swarm_adapter::DialOutcome; use crate::{ConnectionDirection, P2PServer, metrics}; @@ -58,6 +58,9 @@ pub(crate) struct DiscoveryState { /// A handle on the admission policy discovery also runs, so a fork /// boundary can move both with one call. See [`LeanFilter::set_fork_id`]. filter: LeanFilter, + /// The write side of the ENR discovery serves, so a fork boundary can move + /// its `eth2` entry along with the filter. + local_enr_writer: LocalEnrWriter, } impl DiscoveryState { @@ -78,6 +81,7 @@ impl DiscoveryState { local_peer_id, target_peers: spawned.target_peers, filter: spawned.filter, + local_enr_writer: spawned.local_enr_writer, } } @@ -86,6 +90,11 @@ impl DiscoveryState { &self.filter } + /// The write side of the ENR discovery serves. + pub(crate) fn local_enr_writer(&self) -> &LocalEnrWriter { + &self.local_enr_writer + } + /// Connected-peer count above which the loop stops dialing. pub(crate) fn target_peers(&self) -> usize { self.target_peers diff --git a/crates/net/p2p/src/discovery/mod.rs b/crates/net/p2p/src/discovery/mod.rs index 9ca962b9a..5bfd0df39 100644 --- a/crates/net/p2p/src/discovery/mod.rs +++ b/crates/net/p2p/src/discovery/mod.rs @@ -12,9 +12,10 @@ //! [`subnets`] hunt's predicate queries. The casts are what let discovery pace //! its own lookups, since a libp2p connection is otherwise invisible to it. //! -//! Discovery owns this node's ENR once it starts: it re-signs the record when -//! discv5's IP voting moves the address. [`LocalEnr`] reads the record it -//! currently serves, which is what the Beacon API reports. +//! This node's ENR lives in one lock-free slot that discovery serves from. +//! discv5's IP voting re-signs it when the address moves, [`LocalEnrWriter`] +//! re-signs it when a fork boundary moves the `eth2` entry, and [`LocalEnr`] +//! reads it for the Beacon API. //! //! See `docs/discovery.md` for the operator-facing description. @@ -26,18 +27,20 @@ pub(crate) mod subnets; use std::collections::HashSet; use std::net::{IpAddr, SocketAddr}; -use std::sync::{Arc, RwLock}; +use std::sync::Arc; use std::time::Duration; use ethrex_p2p::discovery::{DiscoveryConfig, DiscoveryHandle, DiscoveryServer}; use ethrex_p2p::types::{LocalNode, Node, SharedLocalNode}; +use libssz::SszEncode; +use secp256k1::SecretKey; use tokio::net::UdpSocket; use tokio::sync::watch; use tracing::{info, warn}; use crate::Bootnode; use admission::LeanFilter; -use enr::{EnrForkId, LocalEnrParams, build_local_enr}; +use enr::{ETH2_ENR_KEY, EnrForkId, LocalEnrParams, build_local_enr}; /// Dials per second the loop opens when this node has no peers at all. /// @@ -145,6 +148,9 @@ pub struct SpawnedDiscovery { pub(crate) filter: LeanFilter, /// The ENR discovery serves, read live; see [`LocalEnr`]. pub local_enr: LocalEnr, + /// The same record's write side, which a fork boundary republishes `eth2` + /// through; see [`LocalEnrWriter`]. + pub(crate) local_enr_writer: LocalEnrWriter, /// The attestation subnets this node subscribes to, which its ENR's /// `attnets` advertises. What the subnet hunt keeps supplied with peers. pub(crate) subscription_subnets: HashSet, @@ -168,8 +174,7 @@ pub struct SpawnedDiscovery { pub async fn spawn_discovery( config: DiscoverySpawnConfig, ) -> Result { - let signer = - secp256k1::SecretKey::from_slice(&config.node_key).map_err(DiscoveryError::NodeKey)?; + let signer = SecretKey::from_slice(&config.node_key).map_err(DiscoveryError::NodeKey)?; let bind_addr = SocketAddr::new(config.bind_ip, config.discovery_port); let socket = UdpSocket::bind(bind_addr) @@ -228,9 +233,10 @@ pub async fn spawn_discovery( // re-signs it under `params.signer` whenever IP voting bumps the sequence // number, keeping the extra entries. // - // `shared_local_node` is ethrex's live mirror of that identity: every - // re-signed record is written back into it. [`LocalEnr`] keeps a clone, so - // the identity endpoint reports the record discv5 actually serves. + // `local_node` is the only copy of that identity: discovery serves from it + // and swaps every re-signed record into it. [`LocalEnr`] and + // [`LocalEnrWriter`] keep clones, so the identity endpoint reports the + // record discv5 actually serves and a fork boundary edits that same record. // // `fork_id` publishes an EIP-2124 fork id for discovery to stamp on the ENR // when the chain crosses a fork. Neither chain here keys its peers on an @@ -239,15 +245,18 @@ pub async fn spawn_discovery( // "the publisher is gone" and stops checking. let (fork_id_tx, fork_id_rx) = watch::channel(None); drop(fork_id_tx); - let shared_local_node = Arc::new(RwLock::new(LocalNode { - node: local_node.clone(), - record: local_record.clone(), - })); - let local_enr = LocalEnr(shared_local_node.clone()); + let local_node = SharedLocalNode::new(LocalNode { + node: local_node, + record: local_record, + }); + let local_enr = LocalEnr(local_node.clone()); + let local_enr_writer = LocalEnrWriter { + local_node: local_node.clone(), + signer: params.signer, + }; let server = DiscoveryServer::spawn( local_node, - local_record, params.signer, Arc::new(socket), Box::new(filter.clone()), @@ -266,7 +275,6 @@ pub async fn spawn_discovery( // the whole point of having said it. nat_extip_set: config.advertise_ip.is_some(), }, - shared_local_node, fork_id_rx, ) .await @@ -293,6 +301,7 @@ pub async fn spawn_discovery( handle: discovery, filter, local_enr, + local_enr_writer, subscription_subnets, target_peers: config.target_peers, }) @@ -301,64 +310,99 @@ pub async fn spawn_discovery( /// This node's ENR as discovery serves it right now. /// /// discv5's IP voting re-signs the record at runtime with a new address and a -/// higher `seq`, and ethrex writes every record it re-signs into the -/// [`SharedLocalNode`] it was started with. Reading through that, instead of -/// keeping the string built at startup, is what keeps the Beacon API's -/// `/eth/v1/node/identity` reporting the record peers get over discv5. +/// higher `seq`, and so does [`LocalEnrWriter`] at a fork boundary. Both swap +/// the result into the [`SharedLocalNode`] discovery serves from. Reading +/// through that, instead of keeping the string built at startup, is what keeps +/// the Beacon API's `/eth/v1/node/identity` reporting the record peers get over +/// discv5. /// -/// One direction only: nothing written here would reach discv5, which serves -/// its own copy and overwrites this one on its next edit. See -/// [`update_served_enr`] for the other direction. +/// Read-only, and so safe to hand to the RPC server: editing takes the node +/// key, which only [`LocalEnrWriter`] holds. #[derive(Clone)] pub struct LocalEnr(SharedLocalNode); impl LocalEnr { - /// The served record as an `enr:` string, or `None` if it cannot be read. + /// The served record as an `enr:` string, or `None` if it cannot be + /// encoded. /// /// Encoding cannot fail for a record `spawn_discovery` already encoded, - /// since IP voting edits only the address. A poisoned lock means ethrex - /// panicked mid-edit; this answers `None` rather than spread the panic - /// into whichever caller asked. + /// since the edits after it only replace entries it already carried. + /// Takes no lock: [`SharedLocalNode::load`] is a snapshot. pub fn current(&self) -> Option { - // Cloned out so the guard, which is `!Send`, is dropped right here. - let record = self.0.read().ok()?.record.clone(); - record + self.0 + .load() + .record .enr_url() .inspect_err(|err| warn!(%err, "Could not encode the local ENR")) .ok() } } -/// Publish a new `eth2` entry in the ENR this node serves over discv5. -/// -/// Not possible with the discovery stack this node is built on, so this only -/// says so. [`DiscoveryServer::spawn`] now returns the actor's handle, but -/// `DiscoveryServerProtocol` still has no message that edits the local record: -/// its consumer-facing messages report peer events, prune, ask for dial -/// candidates, run predicate queries and refresh a *remote* ENR. ethrex edits -/// its own record in two places only, both internal: discv5 IP voting -/// (`update_local_ip`, the address) and `refresh_fork_id`, fed by the `fork_id` -/// watch channel `spawn` takes. That one is consumer-driven, but it writes the -/// EIP-2124 `eth` entry, an execution-chain fork id, not the consensus `eth2` -/// entry this node needs moved. Writing into the [`SharedLocalNode`] mirror -/// does not help either: discovery serves its own copy and overwrites the -/// mirror on its next edit. What would close the gap is a protocol message -/// that applies an edit through `NodeRecord::edit` (one `seq` bump, re-signed) -/// and mirrors the result, as `refresh_fork_id` already does for `eth`. +/// The write side of [`LocalEnr`]: edits the record discovery serves. /// -/// Until then the record served to discv5 queries keeps the `eth2` entry it -/// started with, and so does [`LocalEnr`], which reads what discovery serves. Peers already connected are unaffected (they handshake on -/// `Status`, which does follow the switch) and the admission filter does move, -/// but a peer that discovers this node after a boundary reads the stale digest -/// and rejects the record, so a long-running node becomes harder to find until -/// it is restarted. -pub(crate) fn update_served_enr(fork_id: &EnrForkId) { - warn!( - fork_digest = %hex::encode(fork_id.fork_digest), - next_fork_epoch = fork_id.next_fork_epoch, - "The served ENR keeps its startup eth2 entry: the discovery server offers no way to \ - replace its record at runtime" - ); +/// Holds the node key every edit is re-signed with, which is why it is a +/// separate type that stays inside this crate rather than a method on +/// [`LocalEnr`] riding along into the RPC server. +pub(crate) struct LocalEnrWriter { + local_node: SharedLocalNode, + signer: SecretKey, +} + +impl LocalEnrWriter { + /// Serve `fork_id` as this node's `eth2` entry. + /// + /// Does nothing when the served record already carries it. Every edit bumps + /// `seq`, and a bumped `seq` sends each peer that next sees our PING or PONG + /// back for the whole record, so a call that changes nothing must not edit. + /// + /// Otherwise the record is re-signed once with `seq` one higher, and peers + /// that discover this node after a fork boundary read the digest it is on + /// now. Without this they would read the startup digest and reject the + /// record, so a long-running node would grow harder to find with every + /// boundary until it restarted. + pub(crate) fn publish_fork_id(&self, fork_id: &EnrForkId) { + let eth2 = fork_id.to_ssz(); + let served = self.local_node.load(); + let unchanged = served + .record + .pairs() + .extra(ETH2_ENR_KEY) + .is_some_and(|served| served.as_ref() == eth2.as_slice()); + if unchanged { + return; + } + + // The edit runs again on a newer record whenever IP voting swaps one in + // first, so it must come out the same every time: it only replaces the + // one entry, from bytes computed once above. + let edited = self.local_node.edit_record(&self.signer, |pairs| { + let stored = pairs.set_extra(ETH2_ENR_KEY, eth2.clone()); + debug_assert!(stored, "eth2 is outside the ENR dictionary"); + }); + let fork_digest = hex::encode(fork_id.fork_digest); + let next_fork_epoch = fork_id.next_fork_epoch; + match edited { + Ok(local_node) => info!( + %fork_digest, + next_fork_epoch, + enr_seq = local_node.record.seq, + "Serving a new eth2 entry in the local ENR" + ), + Err(err) => warn!( + %err, + %fork_digest, + next_fork_epoch, + "Could not republish the local ENR's eth2 entry; peers keep reading the old one" + ), + } + } + + /// The record discovery serves right now, for tests outside this module + /// that hold a `DiscoveryState` rather than a [`LocalEnr`]. + #[cfg(test)] + pub(crate) fn served_record(&self) -> ethrex_p2p::types::NodeRecord { + self.local_node.load().record.clone() + } } #[cfg(test)] @@ -367,6 +411,7 @@ mod tests { use ethrex_p2p::peer_filter::PeerFilter; use ethrex_p2p::types::NodeRecord; use ethrex_rlp::decode::RLPDecode; + use libssz::SszDecode; use std::net::Ipv4Addr; fn config(discovery_port: u16, advertise_ip: Option) -> DiscoverySpawnConfig { @@ -440,6 +485,56 @@ mod tests { assert_eq!(decode_enr(&local_enr).pairs().ip, Some(advertised)); } + #[tokio::test] + async fn a_new_fork_id_is_what_discovery_serves_next() { + let handle = spawn_discovery(config(0, None)) + .await + .expect("discovery spawns"); + let before = decode_enr(&handle.local_enr.current().expect("the served ENR encodes")); + let next = EnrForkId { + fork_digest: [0x8c, 0x9f, 0x62, 0xfe], + next_fork_version: [0x06, 0x00, 0x00, 0x00], + next_fork_epoch: enr::FAR_FUTURE_EPOCH, + }; + + handle.local_enr_writer.publish_fork_id(&next); + + let after = decode_enr(&handle.local_enr.current().expect("the served ENR encodes")); + let eth2 = after.pairs().extra(ETH2_ENR_KEY).expect("eth2 entry"); + assert_eq!(EnrForkId::from_ssz_bytes(ð2).unwrap(), next); + // One edit, one bump: peers learn of the edit from the higher `seq` in + // our next PING or PONG. + assert_eq!(after.seq, before.seq + 1); + assert!(after.verify_signature(), "re-signed with the node key"); + // Nothing else a peer judges us by moved. + for key in [enr::ATTNETS_ENR_KEY, enr::QUIC_ENR_KEY] { + assert_eq!(after.pairs().extra(key), before.pairs().extra(key)); + } + assert_eq!(after.pairs().ip, before.pairs().ip); + assert_eq!(after.pairs().udp_port, before.pairs().udp_port); + // A peer already on the new digest admits the record. + handle.filter.set_fork_id(next, Vec::new()); + assert!(handle.filter.accepts(&after)); + } + + #[tokio::test] + async fn republishing_the_served_fork_id_leaves_the_record_alone() { + let handle = spawn_discovery(config(0, None)) + .await + .expect("discovery spawns"); + let before = handle.local_enr.current().expect("the served ENR encodes"); + + // `config` publishes `EnrForkId::local()`, and `transition::apply` + // offers the entry on every call, boundary or not. + handle.local_enr_writer.publish_fork_id(&EnrForkId::local()); + + let after = handle.local_enr.current().expect("the served ENR encodes"); + assert_eq!( + after, before, + "no edit, so no `seq` bump for peers to chase" + ); + } + #[tokio::test] async fn spawn_fails_loudly_on_a_busy_port() { let socket = tokio::net::UdpSocket::bind("127.0.0.1:0").await.unwrap(); From 51c4bc93d1438c7a9b009fd155371399ae5cac95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:21:20 -0300 Subject: [PATCH 2/3] docs: say the served ENR follows fork boundaries now The gap the previous commit closes was documented in five places. Two of them used it to explain why the ENR carries no syncnets entry; those now say the record can be edited and nothing writes the set to it yet. --- CLAUDE.md | 10 ++++----- docs/beacon_wire.md | 10 ++++----- docs/discovery.md | 45 +++++++++++++++++++++++++---------------- docs/spec_deviations.md | 7 ++++--- 4 files changed, 42 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 39dde0b86..5ba3b738f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -366,10 +366,10 @@ actual_slot = finalized_slot + 1 + relative_index - Fork-digest rollover happens at runtime (`ForkSchedule`, `beacon::transition`): the next digest's topics are joined `SUBSCRIBE_LEAD_EPOCHS` before its boundary and the previous one's left `UNSUBSCRIBE_LAG_EPOCHS` after, blob-parameter (BPO) boundaries included. - **Known gap**: ethrex's `DiscoveryServer` has no message that edits the served ENR (its one - consumer-driven edit, the `fork_id` watch, writes `eth`, not `eth2`), so its `eth2` entry - keeps the startup digest until a restart; peers discovering the node after a boundary - reject the record. See `discovery::update_served_enr` + Every `apply` also offers the schedule's `ENRForkID` to the served ENR + (`discovery::LocalEnrWriter::publish_fork_id`), which edits it only when it differs, so the + record is re-signed once per boundary and peers discovering the node afterwards read the + digest it is on now - `voluntary_exit`, `proposer_slashing`, `attester_slashing` and `bls_to_execution_change` validate in p2p like aggregates (`gossip::operations`, against the unadvanced head state, which must be electra or fulu: under a gloas head every operation is IGNOREd) and feed @@ -395,7 +395,7 @@ actual_slot = finalized_slot + 1 + relative_index ### Peer Discovery (discv5) - Always on for `beacon` (mainnet bootnode ENRs are not statically dialable, so a crawl is its only way to find a peer); opt-in for `node` behind the lean-only `--discovery.enable`, off by default, so a lean node peers from `--bootnodes` alone and binds no discovery socket. `Network::discovery_enabled` is the one answer; `P2P::spawn` takes `Option` and `P2PServer.discovery` is an `Option`, `None` leaving the dial loop unscheduled - Where it runs: `DEFAULT_DISCOVERY_PORT` (9000) unless `--discovery.port` says otherwise (own UDP socket, must differ from `--gossipsub-port`; checked once by `Options::validate_ports`, which skips the discovery rules when it is off). Co-located nodes that run discovery must each pass `--discovery.port` -- Reuses ethrex's `DiscoveryServer` with discv4 disabled, pinned to a commit of ethrex's `feat/discv5-predicate-queries` (on top of ethrex PR #7217) until both are in a release; `spawn` takes the prepared lean ENR. ethrex re-signs it on IP voting and mirrors it into the `SharedLocalNode` it was spawned with; `discovery::LocalEnr` reads that mirror, so `/eth/v1/node/identity` reports the record discv5 currently serves. Read-only: nothing written there reaches discv5 +- Reuses ethrex's `DiscoveryServer` with discv4 disabled, pinned to a commit of ethrex's `feat/discv5-lock-free-local-enr` (on `feat/discv5-predicate-queries`, on ethrex PR #7217) until all three are in a release; `spawn` takes the prepared lean ENR in a `SharedLocalNode`, a lock-free `ArcSwap` slot that is the only copy of the record. Discovery serves from it and swaps in every record IP voting re-signs; `discovery::LocalEnr` reads it (so `/eth/v1/node/identity` reports the record discv5 currently serves), and `discovery::LocalEnrWriter`, which holds the node key and stays crate-private, edits it by compare-and-swap (`edit_record`: one `seq` bump, re-signed, retried on a concurrent IP update) - ENR follows the beacon phase0 spec: `ip`/`udp`/`quic`/`tcp`/`secp256k1`/`eth2`/`attnets` - Admission mirrors lighthouse: `eth2.fork_digest` must match, `next_fork_*` may differ, a `quic` or `tcp` entry required. Handed to discovery as `LeanFilter: PeerFilter`, so records are judged on arrival, not at dial time; a reject is re-judged on a higher-`seq` ENR - The filter files each admitted peer in `DialTargets`, since discovery hands back a `Node` that carries no `quic`/`tcp`/`attnets`; the dial loop looks the node id back up there diff --git a/docs/beacon_wire.md b/docs/beacon_wire.md index 6c5e9947b..158466df7 100644 --- a/docs/beacon_wire.md +++ b/docs/beacon_wire.md @@ -76,10 +76,10 @@ message's fork comes from its own topic's digest, and a `Status` on either digest is answered, but range sync starts only from a peer on the current digest (or the next one, within clock skew of the boundary). -One gap: the ENR this node serves over discv5 keeps the `eth2` entry it started -with, because ethrex's `DiscoveryServer` offers no way to replace its record at -runtime. Peers already connected are unaffected; peers discovering the node -after a boundary read the stale digest and reject the record. +The ENR this node serves over discv5 moves too: at the boundary its `eth2` +entry is replaced with the new `ENRForkID` and the record is re-signed under a +higher `seq`, so peers discovering the node afterwards read the digest it is on +now. See [discovery.md](./discovery.md#the-enr). ## Gossip @@ -431,7 +431,7 @@ verifications per slot, so nothing is joined without a request. | Piece | Behaviour | | --- | --- | | State | `BeaconWire::sync_committee_subnets`: subnet to `until_epoch`, extended with the later of two requests; ids of 4 or more are dropped | -| Advertised | MetaData v2 and v3 `syncnets` is built from that set, and the sequence number moves whenever the set does. The ENR carries no `syncnets` entry: ethrex's `DiscoveryServer` cannot replace the served record at runtime, the same known gap as the `eth2` entry | +| Advertised | MetaData v2 and v3 `syncnets` is built from that set, and the sequence number moves whenever the set does. The ENR carries no `syncnets` entry: the served record can be edited at runtime now (`discovery::LocalEnrWriter`, which moves `eth2` at a boundary), but nothing writes the subscription set to it | | Left | the 12 s sweep leaves a subnet once the wall epoch reaches its `until_epoch`, and prunes the sync committee pool | | Fork boundaries | `beacon::transition::apply` subscribes the set under each digest it joins and unsubscribes it under each it leaves, like the aggregator attestation subnets | | Validation | the cheap half runs inline, the stateful half on `spawn_blocking` under `SYNC_VALIDATION_PERMITS`, a pool of its own since the burst coincides with the attestations'. An accepted message is pooled with the seats its signature verified; neither it nor a contribution reaches the chain actor | diff --git a/docs/discovery.md b/docs/discovery.md index d17544ed1..eeaf6c5f0 100644 --- a/docs/discovery.md +++ b/docs/discovery.md @@ -85,19 +85,31 @@ discv5 queries. If IP voting later changes our external address, ethrex edits an re-signs that record rather than rebuilding one, so the consensus entries survive the bump; only the sequence number and `ip` move. -`/eth/v1/node/identity` reports the record discovery serves *now*, not the one -logged at startup. ethrex writes every record it re-signs into a shared mirror -(`SharedLocalNode`) it was started with, and the endpoint reads through that -(`discovery::LocalEnr`), so what the Beacon API reports and what peers get over -discv5 are the same bytes even after IP voting. - -The other direction is not possible yet: nothing on this side can change the -record discovery serves. ethrex's discovery protocol has no message that edits -the local record. Its only consumer-driven edit is the EIP-2124 `eth` entry -(an execution-chain fork id), not the `eth2` entry a fork-digest boundary moves, -and writing into the mirror does nothing, since discovery serves its own copy and -overwrites the mirror on its next edit. See the next section for what that costs -at a boundary. +The record lives in one slot shared with ethrex (`SharedLocalNode`, an +`ArcSwap`), and that slot is the only copy: discovery reads it for every PING, +PONG, NODES and handshake it sends, and every edit swaps a whole new record in by +compare-and-swap. Nothing takes a lock. + +| Who | Reads or edits | How | +| --- | --- | --- | +| discv5 IP voting | the `ip` and `udp`/`tcp` entries | ethrex, internally | +| A fork-digest boundary | the `eth2` entry | `discovery::LocalEnrWriter::publish_fork_id`, from `beacon::transition::apply` | +| `/eth/v1/node/identity` | reads the whole record | `discovery::LocalEnr` | + +`/eth/v1/node/identity` therefore reports the record discovery serves *now*, not +the one logged at startup, so what the Beacon API reports and what peers get over +discv5 are the same bytes after IP voting and after a boundary alike. + +An edit is re-signed with `seq` bumped once, and discovery advertises the new +`seq` in its next PING or PONG, which is what sends peers back for the record. If +IP voting swaps a record in between an edit's read and its swap, the edit is +re-applied on top of that record rather than overwriting it, so both survive. +`publish_fork_id` is offered the schedule's `ENRForkID` on every call of +`apply`, boundary or not, and edits only when the served entry differs: a +needless `seq` bump would send every peer back for an identical record. + +The writer holds the node key the record is signed with, so it stays inside the +p2p crate; the RPC server gets only the read side. ## Which peers get dialed @@ -110,10 +122,9 @@ A discovered peer is admitted only if: The digest compared against is the caller's, not a constant: lean passes its hardcoded dummy and `beacon` passes the digest it derived at startup and moves it at each fork-digest boundary, so one admission policy serves both chains. The -record this node serves is not refreshed at a boundary (ethrex's -`DiscoveryServer` offers no way to edit it at runtime; see [The ENR](#the-enr)), -so its `eth2` entry keeps the startup digest until a restart; see -[beacon_wire.md](./beacon_wire.md#the-fork-digest). +record this node serves moves with it: the same call republishes its `eth2` +entry (see [The ENR](#the-enr)), so a peer running these rules keeps admitting +the node after a boundary. A differing `next_fork_version` or `next_fork_epoch` is *not* grounds for rejection: the spec permits connecting to a peer that is incompatible with an diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 191be46cd..654b87fc3 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -640,9 +640,10 @@ message's slot and the signing domain from the config's fork schedule. `until_epoch` (exclusive) clamped to the end of the next period. The node never subscribes without a request, because validating every subnet would cost each follower up to `SYNC_COMMITTEE_SIZE` BLS verifications a slot. - `syncnets` appears in MetaData, never in the ENR: ethrex's `DiscoveryServer` - cannot replace the served record at runtime, the same known gap as the - `eth2` entry. Peers therefore find this node's sync subnets by chance, while + `syncnets` appears in MetaData, never in the ENR: the served record can be + edited at runtime (`discovery::LocalEnrWriter` moves `eth2` at a fork + boundary), but nothing writes the subscription set to it yet. Peers + therefore find this node's sync subnets by chance, while publishing still reaches the mesh, because a joined subnet is subscribed. - The `sync_committee_contribution_and_proof` topic is the exception: it is subscribed under every digest and always validated, and accepted From e8f9089c7f92d12201aa5e3c50d802961d3a20f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:23:25 -0300 Subject: [PATCH 3/3] docs(discovery): IP voting moves only the address entry ethrex's update_local_ip sets ip or ip6 and clears the other; it never touches the udp or tcp ports, which the new table claimed it did. --- docs/discovery.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/discovery.md b/docs/discovery.md index eeaf6c5f0..0e8652247 100644 --- a/docs/discovery.md +++ b/docs/discovery.md @@ -92,7 +92,7 @@ compare-and-swap. Nothing takes a lock. | Who | Reads or edits | How | | --- | --- | --- | -| discv5 IP voting | the `ip` and `udp`/`tcp` entries | ethrex, internally | +| discv5 IP voting | the `ip` (or `ip6`) entry | ethrex, internally | | A fork-digest boundary | the `eth2` entry | `discovery::LocalEnrWriter::publish_fork_id`, from `beacon::transition::apply` | | `/eth/v1/node/identity` | reads the whole record | `discovery::LocalEnr` |