refactor(l1): move the contact table into the discovery server - #7217
refactor(l1): move the contact table into the discovery server#7217MegaRedHand wants to merge 14 commits into
Conversation
The peer table held two unrelated things: the nodes discovery has heard of, and the peers RLPx is connected to. A consumer that only wants discv5 had to spin up an actor carrying `PeerConnection`, `Capability` and a `Store`-backed fork-id filter to get at any of it, and the discv5 tests had to build an in-memory store to test a nonce counter. Contacts, k-buckets, the connection pool and discv5 sessions now live in a `ContactTable` owned outright by `DiscoveryServer` as plain state, so every discv4/discv5 handler reaches them by `&mut self` instead of paying a message hop per inbound packet. The peer table keeps only connected peers, their scores and their request slots. The two halves meet at five points, all of them now pointing one way, from RLPx into discovery: - dial candidates: `next_dial_candidate` returns a `Node`, filtered against a connected set discovery maintains itself - `mark_connected` / `mark_disconnected`, cast alongside the existing `new_connected_peer` and `remove_peer` calls - `set_unwanted` / `set_disposable`, cast from the connection server and the peer handler - lookup pacing, which used to read `target_peers_completion` back out of the peer table and is now computed from discovery's own connected count That last one was the only call pointing from discovery into RLPx; inverting it is what lets discovery run without a peer table at all. Everything crossing the boundary is now a cast except the dial-candidate request, so two actors with sequential mailboxes can never call into each other. Two fixes fell out of the move: - `Contact.session` duplicated the standalone session store and was read only as a fallback, which outlived the disconnect cleanup: a session was never actually dropped for a node that still had a contact. The field is gone and the store is the single source of truth. - `target_reached` was dead, and identical to `target_peers_reached`.
…pping discv5 sessions Two defects found reviewing the contact-table move. `next_dial_candidate` handed the dialer the node from the connection pool. The pool is written once on first sight and never refreshed, while the contact's node is replaced whenever a higher-seq ENR arrives, so the two diverge: a peer first heard of over an unauthenticated discv4 Neighbors packet sits in the pool with whatever endpoint that packet claimed, often TCP port 0, and every later dial used it even after the peer published a signed record correcting the address. `already_tried_peers` clears once the pool is exhausted, so it retried the wrong address indefinitely. The previous code returned the k-bucket contact and only fell back to the pool when the buckets had evicted the id; restore that, and keep the pool entry as the fallback it was. Removing `Contact.session` made the disconnect cleanup effective for the first time, which exposed that it fires too widely: the connection actor's teardown runs for any state that reached `Established`, including attempts rejected during capability negotiation, so an RLPx handshake we turned down for having no `eth` capability destroyed a perfectly good discv5 session and forced a WHOAREYOU round trip to reach a node we could already talk to. The consumer's connection and the discovery session are separate conversations with the same node, so `mark_disconnected` no longer touches sessions. `prune` drops them along with the contact instead, which is both the layer that owns their lifetime and what bounds the store.
Review of the previous commit found the session cleanup it introduced barely fires. `prune` reaps only contacts marked `disposable`, and `set_unwanted` has exactly one caller: the RLPx capability rejection. So for the precise case that commit was written about, a peer we complete a discv5 handshake with and then turn down over RLPx, the contact is marked unwanted, never pruned, and its session is retained for the life of the process. Claiming `prune` bounded the store was wrong. Nor is the contact the right thing to hang the lifetime on. A contact can leave the table without ever being disposable, evicted from a replacement queue by a newer arrival, and a session can be stored for a node whose ENR never parsed into a contact at all, which is the documented reason the store is standalone. Both leave an entry the contact-driven path can no longer reach. That is reachable from outside. The WHOAREYOU rate limiter is keyed on `(ip, src_id)`, so a fresh `src_id` per packet only ever meets the global 100/s cap; each completed handshake orphans an entry. Sessions now carry when they were established and expire from the same prune tick on `SESSION_TTL`, which `Discv5State::session_ips` already used for the other half of the same session. Sharing the constant fixes an asymmetry too: the `session_ips` entry expired after an hour while the keys it guards lived forever, so the IP-rebinding check silently stopped applying to a session that still decrypted. Also from the same review: - `next_dial_candidate` did its bucket lookup before the two set checks rather than after, so the pass that clears `already_tried_peers` walked the whole pool doing O(k) scans, inside the loop that also drains UDP. - `compress_pubkey` moved from `rlpx::utils` to `crate::utils`. It is plain secp256k1 point handling that discv5's handshake needs, and importing it was the last thing tying discovery to the wire protocol. - `DiscoveryServerError::Store` was dead, and put a storage type in discovery's public error enum. - The teardown comment claimed `stopped()` runs even when a handler panics. True of message handlers, false of `started()`, which is where `mark_connected` is sent. - `PeerFilter`'s docs still named the peer table's message loop. - `target_peers_completion` divided by zero on `--p2p.target-peers 0`. The contact table guarded this; the peer table did not. Tests cover the TTL sweep, prune reaching a replacement-list contact, and the pool fallback in `next_dial_candidate`, each verified to fail without its fix. Dropped `a_handle_is_published_once`: it asserted `OnceLock`'s own semantics and paid a real UDP bind and a process-global SIGINT handler in the shared test binary for it.
The consumer had four separate casts for four things it might learn about a peer: `mark_connected`, `mark_disconnected`, `set_unwanted`, `set_disposable`. They now travel as one `update_status(node_id, PeerStatus)`, taking `DiscoveryHandle` from eight methods to five. `PeerStatus` is deliberately a report of one event rather than a state machine, and its docs say so, because the four are not mutually exclusive over a peer's life. `Connected` and `Disconnected` toggle a membership; `Unwanted` and `Disposable` are verdicts that accumulate on the contact and are never cleared. Two rules follow, and getting either wrong is silent: - A verdict never disconnects. `Disposable` is reported by the sync layer for a peer whose connection is still up, when it serves a malformed response, so folding the verdict into the connected set would hand a peer we are actively talking to back to the dialer. - A disconnection never clears a verdict, or hanging up on a peer we had rejected would quietly rehabilitate it. Both are now tested, and each of the three plausible ways to mis-wire the match arm fails one of those tests. Discovery's own `set_disposable` calls, on a UDP send failure, stay as direct calls on the table: `update_status` is the door the consumer comes through, not an internal one.
…isposing contacts Two of the eight states the three peer flags can take were incoherent, and both involved a peer being connected and unwanted at once. `Unwanted` is reported when an RLPx handshake is rejected for capabilities, and that always happens before the connection is registered, so a single connection actor can never produce the pair. Two can: nothing dedupes connection attempts by node id, so while one connection is live a second attempt to the same peer can die on a transient `HandshakeError` and permanently retire a peer we are happily syncing from. Invisibly, since the verdict is only ever read by the dial filter. `update_status` now drops an `Unwanted` report for a node in the connected set. A live connection is better evidence of wantedness than a failed redundant handshake, and putting the rule in the type that owns both pieces of state makes the contradiction unrepresentable rather than merely unlikely. The underlying duplicate-connection race is untouched and still leaves a stale `peers` entry; that is pre-existing and wants a real dedup. The connected-and-disposable case was worse, because it was reached on purpose. `PeerHandler` marked peers disposable on a malformed response, under a comment saying it wanted to "drop the peer rather than just scoring it down". That is not what the flag does. It leaves the connection up and the peer selectable, deletes the peer's Kademlia contact over an eth-protocol fault, leaves it dialable because the dial filter never reads `disposable`, and is erased within five seconds when prune drops the contact and the flag with it. Those three sites now call `record_critical_failure`, which is the peer table's own mechanism for this, is what the same file already uses ninety lines away for the same class of violation, and actually does what the comment claimed. That leaves `disposable` with no sender outside discovery, so `PeerStatus` drops the variant. It now means only what its field doc always said: a contact did not answer us over UDP, which only discovery can observe. Connected-and-disposable stays reachable and is finally coherent, meaning TCP is up while a UDP send failed.
a0984f9 to
f698c6b
Compare
`PeerStatus` invited reading the variants as exclusive states of a peer, which is what made `Unwanted` look like it subsumed `Disconnected`: any standing "we do not want this peer" implies not being connected to it. The variants are not states. They are things that happened, and two of the three were already named that way. `Unwanted` was the odd one out, naming the flag it sets rather than the event that sets it, and it is now `Rejected`: the moment a connection attempt was turned away. The type is `PeerEvent` and the message `record_peer_event`, matching the `record_*` convention the peer table already uses for noting that something happened. Nothing about behaviour changes. The rename does make the disjointness visible that the old naming hid: a handshake is refused before the connection is registered, so `Rejected` is never a transition out of `Connected`, and the two cannot describe the same peer at the same moment.
…e cleanly Nothing deduped connections by node id. Two actors could reach registration for one peer: crossing dials, a peer opening a second socket, or a stale connected set letting us dial someone we already hold. Identity is only known after the handshake, so neither the inbound admission semaphore, which caps count rather than who, nor the initiator's candidate filter can catch it earlier. `new_connected_peer` overwrote on a duplicate key, which left two live sockets behind one table entry. The displaced connection stayed open but invisible to `get_best_peer`, and the first actor to stop then removed the survivor's entry, reported it disconnected to discovery, and freed its broadcaster index. The peer was live, unusable, uncounted, and offered as a dial candidate again. Registration is now a request returning whether the slot was granted. The check and the insert both run in the peer table's message loop, so the claim is atomic between two actors, and the loser hangs up with `DisconnectSent(AlreadyConnected)` rather than registering. That reaches `connection_failed`, which sends the peer a `Disconnect(AlreadyConnected)`: the behaviour devp2p expects of the receiving side, and which this node previously only ever handled on the way in. It also makes the existing "already connected, don't replace it" arm reachable, which until now nothing could construct. Teardown is gated on a new `registered` flag. Without it the loser's `stopped()` hook still releases the winner's registration, which is most of the bug. The peer table keeps no test module since the contact split; this adds one for the claim, including that a slot is released on disconnect so a reconnecting peer is not turned away for the life of the process. Known limit: a half-open connection still holds its slot until the actor notices, and a peer that reconnects in the meantime is refused. Previously it would have displaced the zombie, at the cost of the eviction bug above. Neither handles that case well, and it wants a liveness check the actor runtime does not currently expose.
|
Lines of code reportTotal lines added: Detailed view |
`SESSION_TTL` arrived in #6900 as a bound on `session_ips`, the map recording which IP a session was established from. Evicting an entry there costs nothing on the wire, so an hour, round and safely large, was a fine number to pick. Applying that same constant to the session keys changed what the number buys: a peer we are still talking to loses its keys mid conversation, and the next discv5 message to it pays a WHOAREYOU and a handshake before it is delivered. If that exchange outruns `MESSAGE_CACHE_TIMEOUT`, the message that triggered it is already gone from `pending_by_nonce` and is never retried, so the lookup round is lost outright. What the TTL exists to bound is the peer that handshakes once and never comes back, and nothing about such a peer refreshes anything. So the sweep now measures idle time instead: a session lives as long as it is used, while the handshake-and-vanish entries still go after an hour. Only inbound traffic that decrypted and arrived from the session's own address counts as use. A send would prove no more than that we still want the peer, and refreshing on one would keep alive precisely the sessions this is meant to reap. Both halves of a session are refreshed in the same place, because keys that outlive their `session_ips` entry silently lose the IP-rebinding check in `discv5_handle_ordinary`: a missing entry reads as nothing to compare against. That invariant is why the refresh sits after the match rather than in the decrypt arm, whose sibling arms return through `&mut self`, and it is what the new handler tests pin down, by aging both halves past the TTL and asserting they survive or are reaped together. Known limit: a session in continuous use is never rotated. Bounding how much traffic sits under one set of keys wants a separate maximum lifetime, kept apart from this idle bound so each can be reasoned about on its own.
…e-in-discovery Main's IpPredictor work (#6811) and this branch both rewrote the discovery server's construction, so the conflicts are structural rather than textual: - `DiscoveryConfig` gains both new fields, `target_peers` from here and `nat_extip_set` from main. - `new_for_discv5_test` and the `server.rs` test module keep both sides: main's shared `LocalNode` and IP-predictor tests, and this branch's contact table and `DiscoveryHandle` test. - `dummy_peer_handler` lost its `Store` argument here, because the peer table no longer holds one now that the fork-id filter lives in discovery. Main's new `sync_race_tests` call site drops the argument; the handler it builds never dials, so nothing there wanted the store. - Mergiraf merged `discv5_handlers.rs` imports into a duplicated `lookup` path; folded back into one `discovery::{...}` group with contact_table alongside main's `ip_predictor`.
…e-in-discovery eth/72's sparse blobpool (#6776) and this branch both added a parameter to peer registration: `negotiated_eth`, the eth version actually agreed with the peer, and `is_inbound`. The registration path keeps both, and threads `negotiated_eth` through `do_new_connected_peer` into `PeerData`, which is where the claim this branch introduced does the insert.
🤖 Kimi Code ReviewThis is a substantial refactoring of the discovery/peering architecture with excellent separation of concerns. The PR correctly splits the monolithic
Summary AssessmentCorrectness: High. Race conditions in connection establishment are now properly handled. Detailed Feedback1. Critical: Race Condition Fix in Connection EstablishmentFile: The change from fire-and-forget registration to a claim-based model is correct and fixes a serious bug: // Before: Two actors could overwrite each other's table entry
// After: Atomic claim with bool return
let claimed = state
.peer_table
.new_connected_peer(...)
.await?;
if !claimed {
return Err(PeerConnectionError::DisconnectSent(
DisconnectReason::AlreadyConnected,
));
}
state.registered = true; // Only release if we claimedVerification: The 2. Session Lifecycle ConsistencyFile: The self.contacts.touch_session(&src_id);
if let Some(source) = discv5.session_ips.get_mut(&src_id) {
source.last_used = Instant::now();
}Issue: Ensure these are updated atomically (they are, in the same scope). The comment correctly notes that keys outliving their IP guard would break the rebinding check. 3. Efficiency: Unnecessary Double CloneFile: .choose(&mut rand::rngs::OsRng)
.cloned() // &&Contact -> &Contact
.cloned() // &Contact -> ContactSuggestion: Collect into 4. Architecture: Filter Execution ContextFile: The updated documentation correctly warns that
Security Note: The filter is applied consistently across both discv4 and discv5 paths (see 5. Edge Case: Rejection of Connected PeersFile: PeerEvent::Rejected if self.connected.contains(&node_id) => {
tracing::debug!(... "Ignoring rejection of a peer we are connected to");
}This correctly handles the race where two connection attempts to the same peer result in one succeeding and one failing during handshake. Without this guard, a transient handshake failure would permanently mark a synced peer as unwanted. 6. Memory Safety: Session BoundingFile: The idle-based TTL ( self.sessions
.retain(|_, (_, last_used)| now.saturating_duration_since(*last_used) < SESSION_TTL);Security: Prevents unbounded growth from handshake-only peers. The test 7. Test Utility SimplificationFile: Removing the // Before: dummy_peer_handler(storage).await
// After: dummy_peer_handler().awaitThis was possible because the fork-id filter moved to the discovery layer, decoupling the peer table from storage. 8. Minor: Import OrganizationFile: Moving // discv5 can now use utils::compress_pubkey without importing from rlpx
pub use crate::utils::{compress_pubkey, decompress_pubkey};Nitpicks
ConclusionThis is a well-architected refactoring that eliminates circular dependencies between discovery and the peer table, fixes race conditions in connection handling, and improves testability. The code is production-ready. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
No EVM/gas/RLP/consensus-rule concerns in this diff; the risk is in peer/discovery lifecycle behavior. I couldn’t run the Rust tests here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
…r table Discovery keeps its own set of the peers the consumer is connected to, because it must never call into that consumer: two actors with sequential mailboxes that call each other can deadlock, so every message across the boundary travels inward. The set is fed by `PeerEvent` casts from each connection actor. A mirror can drift where a read cannot. `Connected` is reported from `initialize_connection`, which runs inside `started()`, while the paired `Disconnected` is reported from `stopped()`, and an actor whose `started()` panics never runs the latter. The stranded id is not cosmetic: it inflates `peer_completion`, which is what paces the iterative lookups, so discovery walks the easing curve towards its 10s ceiling while believing it has peers it does not have; and `next_dial_candidate` skips anything in the set, so the node is never dialed again for the life of the process. Neither is recoverable from inside discovery, which has no way to ask. So the consumer pushes the truth. The RLPx initiator already holds both handles and already runs on a timer, so it reads the peer table's connected ids every five seconds and casts them to discovery, which replaces its set and logs when the two disagreed. `connected_peer_ids` returns ids alone rather than reusing `get_connected_peers`, which clones a `PeerConnection` per peer to answer. Five seconds is deliberately slow. This corrects a mirror that is right in every ordinary connect and disconnect; it is not the mechanism that keeps it current, and polling faster would only add traffic to the actor that also drains UDP.
…e client The `Invalid Missing Ancestor Syncing ReOrg` family fails a run or two out of every few, one or two cases at a time, never the same ones twice. Every failure carries the same verdict: `Unable to customize payload: no transactions available for modification`. The case wants the payload CLMocker last built so it can corrupt one transaction field; when that payload has no transactions there is nothing to corrupt and hive abandons the case. No `newPayload`, no `forkchoiceUpdated`, nothing asked of the client, nothing said about it. The empty payload is hive's own. The suite starts an in-process geth as its secondary client and CLMocker rotates payload production between that node and the client under test. geth returns an empty block from `buildPayload` immediately and swaps in the full one only when the background build lands, while CLMocker waits a fixed second before fetching. A loaded runner that pushes the first full build past that second yields the empty fixture. In the `CanonicalReOrg=False` variants the client under test is not even in the picture: it is removed from CLMocker before canonical production, the mock is started with no peers, and the ethrex log captured for such a failure holds no engine call for the whole test. main fails these tests too, on the same code that passes them the next run. So the exclusion is on the verdict hive logged for that case, read from the simulation log, and never on the test name: these tests still fail CI for a wrong `INVALID`, a bad `latestValidHash`, or a sync that never finishes. What was ignored is named in the job output rather than counted, because a case that stops being intermittent should be visible rather than buried in a tally. A case records only byte offsets into the simulation log and the verdict line falls outside them, hence the small helper that scans the log per suite. Upstream: #4610, and #3105 before it.
Motivation
The peer table held two unrelated things: the nodes discovery has heard of, and the peers RLPx is connected to. A consumer that only wants discv5 had to spin up an actor carrying
PeerConnection,Capabilityand aStore-backed fork-id filter to reach any of it. The discv5 tests show the cost plainly:discv5_server_tests.rsbuilt an in-memoryStoreand a fullPeerTableServerto test a nonce counter.Description
Contacts, k-buckets, the connection pool and discv5 sessions move to a
ContactTableowned byDiscoveryServeras plain state, so every discv4/discv5 handler reaches them by&mut selfinstead of paying a message hop per inbound UDP packet.peer_table.rskeeps only connected peers, their scores and their request slots, and drops ~22 protocol methods.The two halves used to meet at five points. All five now point one way, from RLPx into discovery:
get_contact_to_initiateread the pool and the connected peers mapnext_dial_candidate, filtered against a connected set discovery keeps itselfmark_connected/mark_disconnected, cast beside the existingnew_connected_peer/remove_peerset_unwanted/set_disposableon the peer tableremove_peeralso clearedsessionstarget_peers_completionback into the peer tableThat last row was the only call pointing from discovery into RLPx; inverting it is what lets discovery run with no peer table at all. Everything crossing the boundary is now a cast except the dial-candidate request, so two actors with sequential mailboxes can never call into each other.
DiscoveryHandleis how the RLPx side reaches discovery. Discovery starts after theP2PContextthat needs it, and does not start at all when p2p is disabled, so the handle is inert until published and stays inert in that case.Actor traffic, before and after
Before, every actor talked to one
PeerTableServerthat owned both halves of the state. Discovery's own contacts lived there, so discovery was its heaviest caller.The thick red edge is the whole problem:
target_peers_completionis discovery reading a number only an RLPx peer table can produce, and it is what a discv5-only consumer would have had to supply.After, discovery owns its contacts as plain state, so those twenty messages become
&mut selffield access and disappear from the diagram entirely. Nothing leavesDiscoveryServer.What moved, message by message:
new_contacts,new_contact_records,insert_if_newget_contact,get_contact_for_enr_lookup,get_contact_to_revalidate,validate_contactget_closest_nodes,get_closest_from_pool,get_nodes_at_distancesmark_knows_us,record_ping_sent,record_pong_received,record_enr_request_sent,record_enr_response_receivedget_session_info,set_session_infotarget_peers_completionget_contact_to_initiatenext_dial_candidate, RLPxInitiator → DiscoveryServerset_unwantedrecord_peer_event(Rejected)→ DiscoveryServerset_disposableprune_tableprune, internal and Sync → DiscoveryServerrecord_peer_event(Connected | Disconnected)dec_requestsis not shown: it is sent byRequestPermit::drop, so it travels with whoever holds the permit rather than from a fixed actor.Everything the consumer reports about a peer goes through one message,
record_peer_event(node_id, PeerEvent), withPeerEventone ofConnected,Disconnected,Rejected. That takesDiscoveryHandlefrom eight methods to five.They are events rather than states, and are named so that none reads as a standing condition subsuming another: a persistent "unwanted" would imply not being connected, whereas
Rejectednames the moment an attempt was turned away. They are disjoint in practice too, which the earlier naming hid — a handshake is refused before the connection is registered, soRejectedis never a transition out ofConnected.Two rules follow, both tested:
Two properties the second diagram is meant to make checkable at a glance.
DiscoveryServerhas no outgoing edge, so it can run with no peer table in existence. And exactly one edge into it is a call; the other three are casts, so no pair of actors can block on each other.Fixes that fell out of the move
Contact.sessionduplicated the standalone session store and was read only as a fallback, which outlived the disconnect cleanup: a session was never actually dropped for a node that still had a contact. The store is now the single source of truth.target_reachedwas dead, and identical totarget_peers_reached.Two defects caught in review, fixed in the second commit
next_dial_candidatefirst returned the pool node. The pool is written once on first sight and never refreshed, so a peer first seen in an unauthenticated discv4 Neighbors packet kept being dialed at that endpoint (often TCP port 0) even after publishing a signed ENR correcting it, andalready_tried_peersclears on exhaustion so it retried forever. Restored the previous preference for the contact's endpoint, pool as fallback.Established, including attempts rejected during capability negotiation. An RLPx handshake turned down for having noethcapability destroyed a working discv5 session.mark_disconnectedno longer touches sessions;prunedrops them with the contact.Later commits, from a second review round and follow-up
Four more reviewers went at the branch, including one on the fix commit above, which nothing had looked at. The substantive finding: the session cleanup that commit introduced barely fires, because
prunereaps onlydisposablecontacts andset_unwantedhas exactly one caller, the RLPx capability rejection. For the very case the commit was about, the session was retained for the life of the process. It is also reachable from outside, since the WHOAREYOU limiter is keyed(ip, src_id)and a freshsrc_idper packet meets only the global 100/s cap.Sessions now expire on
SESSION_TTLfrom the same prune tick, sharing the constant withDiscv5State::session_ipsso the two halves of one session cannot drift apart. Smaller items in that commit: the lastdiscovery -> rlpximport is gone (compress_pubkeymoved tocrate::utils), a deadStoreErrorvariant left discovery's public error enum, and a comment claimingstopped()always runs was corrected.Writing out the state space then turned up two cells with no sensible meaning, both saying a peer was connected and unwanted at once, and one that was reached deliberately:
record_peer_eventnow drops a rejection for a node in the connected set, which makes the state unrepresentable rather than merely unlikely.disposabledoes not do that: it leaves the connection up and the peer selectable, deletes the peer's Kademlia contact over an eth-protocol fault, leaves it dialable because the dial filter never reads the flag, and is erased by the next prune. Those sites now callrecord_critical_failure, which is what the same file already used ninety lines away for the same class of violation. With no external sender left,DisposableleavesPeerEventand the flag goes back to meaning only what its doc always said: a contact did not answer us over UDP.Duplicate connections lose cleanly
Nothing deduped connections by node id, which is the race that made connected-and-unwanted reachable, and it had a second symptom.
new_connected_peeroverwrote on a duplicate key, so two live sockets shared one table entry: the displaced connection stayed open but invisible toget_best_peer, and the first actor to stop then removed the survivor's entry, reported it disconnected, and freed its broadcaster index.Registration is now a request returning whether the slot was granted. The check and the insert both run in the peer table's message loop, so the claim is atomic between actors; the loser hangs up with
DisconnectSent(AlreadyConnected), which sends the peer the disconnect devp2p expects of the receiving side and which this node previously only ever handled inbound. Teardown is gated on a newregisteredflag, without which the loser'sstopped()still releases the winner's registration.Known gaps, not fixed here
admin_addPeeris reachable from the RPC server beforestart_networkpublishes the discovery handle. A call inside that window establishes a connection whoseConnectedreport is dropped, leaving the node undercounted for its lifetime and dialable again. Needs an operator call in a window of roughly ten milliseconds, and the duplicate dial is now refused by the claim.started()hook skipsstopped()entirely, so it strands an id in the connected set. It strands the peer table'speersentry identically, so the two stores stay consistent with each other; a reconcile would have to cover both.METRICSsingleton straddles both halves.EthForkIdFiltersharing a module with thePeerFiltertrait, andTARGET_PEERSliving inpeer_table.rs, are the easy remainder.Testing
mainis merged into the branch as of5962a33cc; three files were touched by both sides, including #7204 landing inside a function this branch rewrote, so the auto-merge was checked by hand rather than trusted.cargo test -p ethrex-p2p -p ethrex-rpcplus the full integration suite: 124 / 119 / 969, all passing, clippy clean. New tests cover the connected-set lifecycle, session lifetime and its TTL sweep, prune reaching a replacement-list contact, both arms of the dial-endpoint choice, an unpublishedDiscoveryHandlestaying inert, a rejection being ignored for a connected peer but landing for a disconnected one, and the registration claim including that a slot is released on disconnect. Each regression test was checked against a mutant of its own fix rather than assumed to fail.Not done here
There is no periodic reconcile of the connected set against the peer table. Given the windows listed above, and that both leave the peer table's own map equally stale, a reconcile is worth adding only if either turns out to happen in practice.