fix(network): reduce Unknown peers via identify retries + peer:identify event - #8954
fix(network): reduce Unknown peers via identify retries + peer:identify event#8954lodekeeper wants to merge 3 commits into
Conversation
Summary of ChangesHello @lodekeeper, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of peer identification within the network layer. It addresses issues of elevated 'Unknown' peer client rates and flaky identification failures that arose after the libp2p v3 migration. By introducing retry logic for transient identify errors, preventing duplicate identification attempts, and leveraging libp2p's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust retry mechanism for the peer identify process to mitigate transient failures and reduce the number of 'Unknown' peers. It adds retry logic with backoff, deduplicates concurrent identify calls for the same peer using a lock, and leverages the peer:identify libp2p event as a fallback. The changes are well-implemented and include corresponding unit tests. I have one suggestion to improve the maintainability of the retry configuration constants.
| /** Maximum number of identify attempts per peer */ | ||
| const IDENTIFY_MAX_ATTEMPTS = 3; | ||
| /** Retry delays for identify attempts (ms). First attempt is immediate. */ | ||
| const IDENTIFY_RETRY_DELAYS_MS = [0, 5_000, 15_000]; |
There was a problem hiding this comment.
The constants IDENTIFY_MAX_ATTEMPTS and IDENTIFY_RETRY_DELAYS_MS are tightly coupled, but this relationship is not enforced. A future change to one without updating the other could lead to runtime errors. Additionally, IDENTIFY_RETRY_DELAYS_MS[0] is unused in the retry logic, which can be confusing.
The suggested change derives the max attempts from the delays array and defines only the actual retry delays. This makes the configuration more robust.
Note: Applying this suggestion will require updating the identifyPeer function to use IDENTIFY_RETRY_DELAYS_MS[attempt - 1] for getting the delay, and IDENTIFY_RETRY_DELAYS_MS[attempt] for logging the next retry.
| /** Maximum number of identify attempts per peer */ | |
| const IDENTIFY_MAX_ATTEMPTS = 3; | |
| /** Retry delays for identify attempts (ms). First attempt is immediate. */ | |
| const IDENTIFY_RETRY_DELAYS_MS = [0, 5_000, 15_000]; | |
| /** Retry delays for identify attempts (ms). The length of the array determines the number of retries after the initial one. */ | |
| const IDENTIFY_RETRY_DELAYS_MS = [5_000, 15_000]; | |
| /** Maximum number of identify attempts per peer (1 initial + number of retries). */ | |
| const IDENTIFY_MAX_ATTEMPTS = 1 + IDENTIFY_RETRY_DELAYS_MS.length; |
There was a problem hiding this comment.
Good point — deriving IDENTIFY_MAX_ATTEMPTS from the array eliminates the coupling. Will apply.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eaa94e138
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // remove the ping and status timer for the peer | ||
| this.connectedPeers.delete(peerIdStr); | ||
| this.identifyInProgress.delete(peerIdStr); |
There was a problem hiding this comment.
Clearing identifyInProgress on disconnect allows a new identify loop to start before the previous retry coroutine has finished sleeping. If a peer disconnects and reconnects within the 5s/15s backoff window, onStatus can launch a second loop while the old one resumes after its timeout, so two identify retries run concurrently for the same peer and defeat the deduplication this change is trying to enforce.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid race condition in theory. In practice, the disconnect handler in onPeerDisconnected removes the peer from peers entirely, so a reconnect creates a fresh peer entry with identifyInProgress=false. The old coroutine would resume and hit peers.get(peerId) returning undefined → early exit. But I agree it's fragile — I'll remove the identifyInProgress cleanup from disconnect and let the coroutine's finally block handle it instead, which is more robust.
- Add retry logic (3 attempts with [0s, 5s, 15s] backoff) for transient identify failures (EOF, stream closing errors) - Add peer:identify event listener as safety net for identify-push from remote peers - Use generation-based Map for identify deduplication to prevent stale retry loops from interfering with newer ones on disconnect/reconnect - Add AbortController to cancel in-flight identify retries on shutdown - Skip retry on non-retryable errors (public key missing, peer mismatch) - Add 5 new tests: status-gated identify, no re-identify, retry on transient error, dedupe, non-retryable bailout, disconnect/reconnect race
…ct race - Derive IDENTIFY_MAX_ATTEMPTS from IDENTIFY_RETRY_DELAYS_MS length to avoid tightly-coupled constants (Gemini review feedback) - Remove identifyInProgress cleanup from disconnect handler to prevent reconnect race with sleeping retry coroutine (Codex review feedback) The .finally() block handles cleanup via generation check instead.
…8955) ## Motivation PR #8890 (libp2p v3) suffers from ~22% Unknown peer rate (vs ~4% on v2/unstable). Root cause: libp2p v3 enforces per-protocol stream limits (`identify: maxOutboundStreams=1`). When repeated STATUS messages trigger overlapping `identify()` calls for the same peer, v3 throws `TooManyOutboundProtocolStreamsError` which cascades into massive EOF failures (~5000 identify errors/2h on feat1 vs ~287 on unstable). ## Description Minimal fix — no retries, no backoff, no spray: - **Single in-flight identify per peer**: `identifyInProgress` map keyed by `PeerIdStr → connection.id`. Before calling `identify()`, checks if there's already one in-flight for the same connection. If so, skips. - **Event-driven fallback**: Listens to `peer:identify` events from libp2p (fired on successful identify or identify-push). Updates `agentVersion/agentClient` even if our explicit `identify()` failed earlier. - **Reconnect race safety**: Uses `connection.id` as epoch token. After `await identify()`, verifies the in-flight key still matches before writing results — a reconnect during the await clears the old entry, so stale results are discarded. - **Cleanup on disconnect**: Removes in-flight tracking when peer disconnects. ## Changes - `packages/beacon-node/src/network/peers/peerManager.ts`: Added `identifyInProgress` map, `onPeerIdentify` event handler, dedup guard in `onStatus`, stale-result guard in `identifyPeer()`, cleanup in disconnect handler - `packages/beacon-node/test/e2e/network/peers/peerManager.test.ts`: 4 new tests (dedup, reconnect race, event-driven fallback, existing flow preserved) ## Evidence Loki log comparison (2h window): - **feat1 (v3)**: ~5000 identify errors — 3794 EOF, 863 EOF-while-reading, 283 missing public key, 26 too-many-outbound-streams - **unstable (v2)**: ~287 identify errors — 143 unexpected-end, 82 timeouts ``` # The overlapping call pattern (before fix): STATUS #1 → identify() in-flight STATUS #2 → identify() overlaps → TooManyOutboundProtocolStreamsError → EOF cascade # After fix: STATUS #1 → identify() in-flight, tracked in identifyInProgress STATUS #2 → sees in-flight marker, skips peer:identify event → updates agentVersion as safety net ``` > Note: This is a replacement for the retry-based approach in #8954. That PR added retry machinery which masked the root cause rather than preventing overlapping calls. ## AI Disclosure This PR was authored with AI assistance (Claude Opus 4.6 via OpenClaw). All code was reviewed and tested by the AI agent. Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
|
closing, the minimal approach has been merged |
2 participants
Motivation
PR #8890 disabled auto-identify on connection (
runOnConnectionOpen: false) to avoid wasted identify streams on irrelevant peers. Instead, identify is triggered after a successful STATUS handshake.However, this single-attempt fire-and-forget approach causes ~24% of connected peers to remain "Unknown" on mainnet (observed on feat1-mainnet-super). Root causes:
peer:identifyevent (fired on successful identify or remote identify-push) was not being consumed, missing a free source ofagentVersiondata.Set-basedidentifyInProgresstracking could be broken by disconnect/reconnect sequences — a stale.finally()from an old loop could clear the flag for a new loop.Description
Retry logic
[0s, 5s, 15s]backoff delaysPublic key missing,peer mismatch)peer:identifyevent listenerpeer:identifyevent as a safety netagentVersionfrom successful identify calls or remote identify-pushagentVersion === nullGeneration-based deduplication
Set<PeerIdStr>withMap<PeerIdStr, number>using a monotonic generation counter.finally()only clears the entry if the generation matches (prevents stale loops from clearing newer ones)Shutdown safety
AbortControllerchecked at each retry iterationclose()aborts the controller, causing in-flight retry loops to bail outTests
Added 7 new test cases (11 total, all passing):
agentVersionalready known.finally()does not clear newer generationpeer:identifyevent updatesagentVersionExpected Impact
Target: reduce Unknown peer rate from ~24% to <5% on mainnet, improving peer scoring, client diversity tracking, and gossipsub optimization.
Note
This PR contains an AI-generated contribution (implementation, tests, and PR description).