Update peer endpoints

[mpetrunic]

Breaking:
peer.address -> peer.last_seen_p2p_address

Added clarifications and examples from #97

@AgeManning is this ok?

resolves #97

[mcdee]

Do we really want pagination in this API? What is the use case for fetching an arbitrary subset of peers?

[mpetrunic]

Do we really want pagination in this API? What is the use case for fetching an arbitrary subset of peers?

you are suppose to include disconnected peers as well, it could be response with 500-600 peers or even more on mainnet

[mcdee]

Surely that is an argument to have filtering, not pagination.

[mpetrunic]

Surely that is an argument to have filtering, not pagination.

On what fields would you filter?
I guess, you can always set limit to 10k and get all results.

Let's say I want all disconnected outgoing peers, that could still be 300 responses

[mcdee]

Let's say I want all disconnected outgoing peers, that could still be 300 responses

Yes, but if that is the information you want then why receive it piecemeal going through multiple requests rather than all in one response?

From my experience, pagination is useful when results are ordered and end-user facing. For example, if someone wanted to list validators ordered by balance it's possible they are doing so to just see the first x validators, in which pagination makes sense because the user may well stop at page 1 or 2 of several hundred. However, if the common requirement is to obtain all results, possibly restricted by filter, I don't see the benefit of doing this.

A good example of where this caused problems was in the beacon chain explorers recently. They use the prysm API to fetch validators, and it's paginated. The APi is by default limited to 250 responses, so the explorer required around 300 calls to obtain the data. The problem was that fetching validators for a non-head state required recreating the state from the last checkpoint, so what happened was:

• the beacon node recreated the state for all 70-odd thousand validators (which was taking up to 1 minute)
• the beacon node returned 250 of the validators
• the rest of the results were discarded

This caused the explorers to be unable to provide information. It could be possible to cache the data, but then you're in to a world of second-guessing which results are going to be requested again, cache management, and all that fun.

All that said, if client teams want to build pagination in to their APIs that's up to them. But if it's going to happen I'd far rather it was purely optional, and standard across all APIs (perhaps by adding generic pageSize and cursor options to all queries). The default should always be to return all requested values.

[AgeManning]

Originally I was concerned about there not being an endpoint for peer count.

I imagine there are going to be a lot of applications wanting to know how many connected peers a client has. If I've understood the current API's correctly, in order to do that, they have to receive all the data for all peers the client currently knows about and then filter. I would imagine they would poll regularly for this.

Perhaps we have a peer_count endpoint which can have the queries connected, disconnected etc, which returns a single uint.

[mcdee]

Perhaps we have a peer_count endpoint which can have the queries connected, disconnected etc, which returns a single uint.

Or not take a query and return the number in each state, which could be easier on the implementer.

[realbigsean]

I don't really see a need for ordering by peer_id + pagination. Ordering by a time-based metric like a new state_last_updated or first_connected field might make sense. But overall I'd probably be in favor of no ordering/pagination (unless optionally implemented by client teams) and instead adding a state filter and a new peer_count endpoint.

[mpetrunic]

As per comments here, removed pagination and peer ordering. I've added peer filtering by connection direction and state.

Since we cannot really have /node/peers/peer_count since peer_count would be peer id in that case, I've added peer_count (number of connected peers) to /node/indentity. I'm not too happy by that solution so if you have better proposal, I can revert last commit and do that instead.

[AgeManning]

What about /node/peer_count ?

Or maybe we could add it to health?

I'm not too concerned either way. Just some suggestions. I assume its something that would get polled regularly for misc applications.

[mcdee]

A separate /eth/v1/node/peer_counts seems to follow the existing API endpoint layout most closely. A per-state count would be useful, e.g.:

{
 "data": {
    "disconnected": "12",
    "connecting": "34",
    "connected": "56",
    "disconnecting": "78"
  }
}

(As an aside, we should specify all possible states somewhere).

[djrtwo]

looks good, quick question and a minor copy suggestion

[protolambda]

@mpetrunic I still see a meta field in the peers list endpoint with a count in it, is that a remnant from the removed peers pagination? Shouldn't it always be equal to the length of the returned json list?

[mpetrunic]

@mpetrunic I still see a meta field in the peers list endpoint with a count in it, is that a remnant from the removed peers pagination? Shouldn't it always be equal to the length of the returned json list?

Seems like I forgot to remove it. Yeah it should always be length of list