-
Notifications
You must be signed in to change notification settings - Fork 220
Extend /eth/v1/node/peers with peer scoring and disconnect reasons #606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
bbe5c26
6d0a00f
cff06ce
5f8cfdd
48e7fbf
06c5b49
b609679
1568375
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,33 @@ Peer: | |
| $ref: "./p2p.yaml#/PeerConnectionState" | ||
| direction: | ||
| $ref: "./p2p.yaml#/PeerConnectionDirection" | ||
| agent_version: | ||
| type: string | ||
| description: | | ||
| The peer's libp2p agent version string (from the libp2p identify | ||
| protocol). OPTIONAL; clients MAY omit if not yet observed. | ||
| example: "Lighthouse/v8.1.3-66919c2/aarch64-linux" | ||
| score: | ||
| type: number | ||
| format: double | ||
| description: | | ||
| Client-native peer score. OPTIONAL. | ||
| This number is not consistent between clients, | ||
| but can be compared to other peers on the same client. | ||
| example: -17.4828 | ||
| disconnect_reason: | ||
| description: | | ||
| Reason the client last disconnected from this peer. OPTIONAL. | ||
| This field MUST only be set if the connection status is disconnected or disconnecting. | ||
| $ref: "./p2p.yaml#/PeerDisconnectReason" | ||
| downscore_reasons: | ||
| type: array | ||
| description: | | ||
| Reasons that the client has been down scored in their current session. OPTIONAL. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this supposed to be added per occurrence, or once if it occurred at least once? E.g., if ["gossip_invalid_block", "gossip_invalid_block"]or ["gossip_invalid_block"]I assume the latter, but it might be worth clarifying this in the description.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would expect the latter. The description was a mess so i reduced it substantially because it was ai word slop... can potentially describe it as a distinct set... |
||
| SHOULD be a distinct set of downscare reasons. | ||
| items: | ||
| $ref: "./p2p.yaml#/PeerScoreReason" | ||
| example: ["rpc_bad_blocks_by_range", "rpc_rate_limited"] | ||
|
rolfyone marked this conversation as resolved.
|
||
|
|
||
| PeerConnectionState: | ||
| type: string | ||
|
|
@@ -80,3 +107,43 @@ Multiaddr: | |
| type: string | ||
| description: "[Read more](https://docs.libp2p.io/reference/glossary/#multiaddr)" | ||
| example: "/ip4/7.7.7.7/tcp/4242/p2p/QmYyQSo1c1Ym7orWxLYvCrM2EmxFTANf8wXmmE7DWjhx5N" | ||
|
|
||
| PeerScoreReason: | ||
| type: string | ||
| description: | | ||
| Limited set of peer downscore reasons that may be returned. | ||
| Implementations populating `Peer.downscore_reasons` MUST | ||
| emit values from this list. Consumers SHOULD tolerate unknown | ||
| values for forward compatibility. | ||
| example: | ||
| - rpc_invalid_request | ||
| - rpc_invalid_response | ||
| - rpc_rate_limited | ||
| - rpc_timeout | ||
| - rpc_io_error | ||
| - rpc_bad_blocks_by_range | ||
| - rpc_bad_blocks_by_root | ||
| - gossip_invalid_block | ||
| - gossip_invalid_attestation | ||
| - gossip_invalid_blob_sidecar | ||
| - gossip_invalid_data_column_sidecar | ||
| - sync_bad_batch | ||
| - status_unviable_fork | ||
| - behaviour_penalty | ||
| - unknown | ||
|
|
||
| PeerDisconnectReason: | ||
| type: string | ||
| description: | | ||
| Implementations populating `Peer.disconnect_reason` MUST emit | ||
| values from this list. Consumers SHOULD tolerate unknown values for | ||
| forward compatibility. | ||
| example: | ||
| - bad_score | ||
| - irrelevant_network | ||
| - unviable_fork | ||
| - too_many_peers | ||
| - rate_limited | ||
| - io_error | ||
| - client_shutdown | ||
| - unknown | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above, consider using |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might wanna do a v2 instead of adding new fields, but curious what others think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im happy with new fields as long as they're not mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah none of this should be mandatory.
if we ever do a v2 we should have these mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this specific api it's fine to add new optional fields, it's also more of a debug api, but mostly it's fine because we don't or rather can't support ssz here because of the format/return values, I just wanna make it clear that this is not a general pattern we wanna have, or encourage