feat(beacon-api): expose peer-scoring fields on /eth/v1/node/peers - #9373
Draft
barnabasbusa wants to merge 4 commits into
Draft
feat(beacon-api): expose peer-scoring fields on /eth/v1/node/peers#9373barnabasbusa wants to merge 4 commits into
barnabasbusa wants to merge 4 commits into
Conversation
Adds three fields to PeerScoreStat returned by GET /eth/v1/lodestar/lodestar_peer_score_stats: - lastActionName: the action label passed to reportPeer - lastActionDeltaScore: post-clamp score change of the last action - lastActionUnixMs: timestamp of the last action This lets external tooling display per-peer downscore reasons (e.g. "Lodestar downscored Lighthouse for invalid_request") without having to scrape Prometheus labels or logs. Captured in RealScore.add via an optional actionName parameter; only explicit reportPeer paths set it, so the gossipsub heartbeat decay path does not clobber the metadata.
Adds four optional fields to the NodePeer payload returned by
GET /eth/v1/node/peers and GET /eth/v1/node/peers/{peer_id}:
- agent_version: libp2p identify agent string
- score: the composite lodestar score for the peer
- disconnect_reason: controlled vocab last-disconnect reason (always
omitted by lodestar for now; left in the type for spec parity)
- downscore_reasons: controlled vocab `PeerScoreReason` derived from
the most recent reportPeer action recorded by PeerRpcScoreStore
The fields are emitted as snake_case keys alongside the existing peer
fields, mirroring the WIP beacon-API peer-scoring extension. They are
omitted entirely when no data is available so existing consumers see
no schema change.
Implementation notes:
- NodePeer keeps its SSZ container for the core spec fields and gains
TS-only optional extras; getPeers/getPeer override toJson/fromJson
to round-trip the extras without inventing new SSZ types.
- PeerRpcScoreStore.getStatByPeerId surfaces the per-peer stat without
inserting a default entry, so the read path stays side-effect free.
- mapPeerScoreReason translates lodestar's reportPeer actionName
strings (RequestErrorCode values + gossip/sync labels) to the
controlled vocab so external tooling does not need to know
lodestar-specific identifiers.
…disconnect_reason Adds a bounded `lastDisconnectByPeerId` map to `PeerManager` that captures the most recent goodbye code observed for each peer, both for outbound goodbyes (sent by lodestar via `goodbyeAndDisconnect`) and inbound ones (received via the GOODBYE RPC handler). Inbound disconnects without a goodbye (e.g. Nimbus) are recorded as `INBOUND_DISCONNECT` so the API surface is non-empty for those peers too. `networkCore._dumpPeer` now reads this via `peerManager.getLastDisconnect` and maps the `GoodByeReasonCode` to the controlled `PeerDisconnectReason` vocabulary (`client_shutdown`, `irrelevant_network`, `io_error`, `unviable_fork`, `too_many_peers`, `bad_score`, `inbound_disconnect`, `unknown`) before placing it on the `disconnect_reason` field added in the prior commit. The map is bounded to 1024 entries with FIFO eviction to cap memory usage while still surfacing recent disconnects for tooling that polls `/eth/v1/node/peers`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the peer-scoring extension for the beacon API, adding fields such as agent_version, score, disconnect_reason, and downscore_reasons to the /eth/v1/node/peers endpoint. The feedback identifies a logic issue where stale disconnection reasons are not cleared upon peer reconnection, which could lead to the API reporting outdated information. Additionally, it is suggested to only surface the disconnect_reason for peers that are not currently connected to improve the clarity of the response.
Comment on lines
+899
to
+901
| if (!this.lastDisconnectByPeerId.has(peerIdStr)) { | ||
| this.recordDisconnect(peerIdStr, GoodByeReasonCode.INBOUND_DISCONNECT, false); | ||
| } |
Contributor
There was a problem hiding this comment.
There is a logic issue here because lastDisconnectByPeerId is not cleared when a peer reconnects.
- If a peer previously disconnected with a specific reason (e.g.,
TOO_MANY_PEERS), that entry remains in the map. - If the peer reconnects and later drops the connection without a goodbye (an inbound disconnect), the check
!this.lastDisconnectByPeerId.has(peerIdStr)will befalsebecause of the stale entry from the previous session. - Consequently, the new
INBOUND_DISCONNECTwill not be recorded, and the API will continue to report the old reason.
To fix this, this.lastDisconnectByPeerId.delete(peerIdStr) should be called in trackLibp2pConnection when a new peer session starts.
| const downscoreReasons = | ||
| scoreStat && scoreStat.lastActionName !== null ? [mapPeerScoreReason(scoreStat.lastActionName)] : undefined; | ||
| const lastDisconnect = this.peerManager.getLastDisconnect(peerIdStr); | ||
| const disconnectReason = lastDisconnect ? mapDisconnectReason(lastDisconnect.code) : undefined; |
Contributor
There was a problem hiding this comment.
Surfacing a disconnect_reason for a peer that is currently connected is confusing, as it would represent a reason from a previous session. It is better to only include this field when the peer is not in a connected state.
const isConnected = connections.some((c) => c.status === "open");
const disconnectReason = !isConnected && lastDisconnect ? mapDisconnectReason(lastDisconnect.code) : undefined;… state Per the proposed beacon-API spec (ethereum/beacon-APIs#606), `disconnect_reason` MUST only be populated when the peer's `state` is `disconnected` or `disconnecting`. Compute the node-peer view first and only attach a mapped `disconnectReason` when the resolved state matches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends
/eth/v1/node/peers(and/eth/v1/node/peers/{peer_id}) with four optional fields per a simplified beacon-API spec extension currently under discussion:agent_version— from libp2p peerstorescore— composite score viaPeerRpcScoreStore.getScore(peerId)disconnect_reason— from new boundedlastDisconnectByPeerIdmap onPeerManagerdownscore_reasons— single-element array derived from existinglastActionNameSpec proposal
ethereum/beacon-APIs#606
What's in this PR
packages/api/src/beacon/routes/node.ts) — extendsNodePeerwith 4 optional fields plus customnodePeerToJson/nodePeerFromJsonround-tripping (snake_case wire names).packages/beacon-node/src/network/peers/score/{interface,store}.ts) — non-mutatinggetStatByPeerId().packages/beacon-node/src/network/peers/peerManager.ts) — boundedlastDisconnectByPeerIdmap (FIFO, 1024) recorded on outboundgoodbyeAndDisconnect, inboundonGoodbye, and bare libp2ponLibp2pPeerDisconnect(covers peers that drop without goodbye).packages/beacon-node/src/api/impl/node/utils.ts) —mapPeerScoreReasonandmapDisconnectReason._dumpPeer(packages/beacon-node/src/network/core/networkCore.ts).Note on
inbound_disconnectThis implementation surfaces a non-spec value
inbound_disconnectwhen the peer drops the connection without sending goodbye. Worth discussing in #606 whether this should be added to the controlled vocab.Coordinated implementations
Part of a coordinated multi-client effort — see ethereum/beacon-APIs#606 for the other five client PRs.
Status
Draft.
pnpm build+pnpm check-typespass. Test suite not yet exercised end-to-end against the new fields; opening for cross-client coordination first.