Skip to content
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

Inconsistency in /eth/v1/node/peers #366

Open
mcdee opened this issue Oct 21, 2023 · 5 comments · May be fixed by #388
Open

Inconsistency in /eth/v1/node/peers #366

mcdee opened this issue Oct 21, 2023 · 5 comments · May be fixed by #388

Comments

@mcdee
Copy link
Contributor

mcdee commented Oct 21, 2023

The /eth/v1/node/peers returns a meta element that contains a count item. The schema for this item declares the value to be a number, which is inconsistent with the common format of providing numeric values as quoted strings.

A quick tour of the main beacon nodes shows the following for this value:

  • lighthouse: returns the value unquoted
  • nimbus: returns the value quoted
  • prysm: does not return the meta element
  • teku: returns the value unquoted

Options that I can think of:

  1. patch the spec so that the v1 endpoint returns a quoted value
  2. raise bugs with the relevant beacon nodes to ensure this value is returned unquoted
  3. deprecate the v1 endpoint and create a v2 endpoint with a quoted value
    1. retaining the meta element
    2. discarding the meta element (nowhere else do we return a value like this that can easily be calculated from the other returned information)
    3. moving the count value to the top level returned object where all other metadata lives

My inclination is for option 3 suboption 2, but I would be interested in hearing if anyone has a reason for this meta element to exist, and the count value within it.

@rolfyone
Copy link
Collaborator

i wouldn't be against a v2 without the count - its pretty trivial to get the count from an array...

I guess that translates to 3 suboption 2 being my preference.

@arnetheduck
Copy link
Contributor

We will be fixing the count in nimbus - ie whether or not a v2 happens is orthogonal to whether v1 is correctly implemented in clients as far as I'm concerned.

@nflaig
Copy link
Collaborator

nflaig commented Dec 5, 2023

The meta element might have been a good idea if used consistently across all APIs but as it is right now, any top-level property should be considered metadata (execution_optimistic, finalized, etc.) and everything else (the actual data) must be part of data element.

As this API is an outlier and does not follow this pattern, I would also prefer option 3 suboption 2.

@dapplion
Copy link
Collaborator

dapplion commented Dec 6, 2023

+1 to burn the meta

@dapplion
Copy link
Collaborator

No one likes the meta element, but there's no appetite either for the effort to remove it

According to @mcdee review of clients seems that support of the meta element is poor. If no-one has complained should mean that no consumer is dependent on it.

To address this one-off oddity we could do a breaking change on route v1 and mark the meta property as optional. Then only Nimbus needs to fix the route to be compliant, which @arnetheduck is already keen on doing.

Thoughts?

@dapplion dapplion linked a pull request Dec 12, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants