diff --git a/CLAUDE.md b/CLAUDE.md index 03464c28e..be3d1c300 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/Cargo.lock b/Cargo.lock index 01a789b60..702f01ab4 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" @@ -1916,7 +1925,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]] @@ -2300,7 +2309,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", @@ -2325,7 +2334,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", @@ -2360,7 +2369,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", @@ -2382,7 +2391,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", @@ -2405,7 +2414,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", @@ -2423,7 +2432,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", @@ -2441,7 +2450,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", @@ -2453,10 +2462,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", @@ -2495,7 +2505,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", @@ -2505,7 +2515,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", @@ -2528,7 +2538,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", @@ -2548,7 +2558,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", @@ -6059,7 +6069,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -6764,7 +6774,7 @@ dependencies = [ "getrandom 0.4.2", "once_cell", "rustix 1.1.4", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -7551,7 +7561,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 c05c6b9f9..4d85f3498 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -64,20 +64,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 c6e3b8652..1684f14d1 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(); diff --git a/docs/beacon_wire.md b/docs/beacon_wire.md index c2138785b..a73ec3c2a 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 @@ -442,7 +442,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..0e8652247 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` (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` | + +`/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 49ee15c03..1ff1ddc2a 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -673,9 +673,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