Conversation
6fe82bf to
e0ea5c8
Compare
|
Similar to griffiti watermarks, it's reasonable to expect that there are nodes that either can't or choose not to pass back version specifics..... It's also seen sometimes in these watermarks that only one client (typically the CL) is declared (harder to tell if its unknown or no space). |
|
I consider this to be quite different from graffiti data since that is public, whereas the Beacon API is not generally exposed publicly. I don't really see an issue with passing back precise version information here. On the current V1 endpoint most clients already include everything that is proposed in this PR for the CL side, just in a less structured "User-agent" way:
I can see the beacon node not having EL-side information for some reason (e.g. because the EL client is not responding to the beacon node's Engine API requests). In such cases we could either not include the "execution_client" field in the return data (and make that entire field optional), or explicitly define an For the usecase I'm looking for, just the client names would suffice, e.g. this node is running Lodestar+Reth. But since most clients already include much more in their current user-agent, I decided to just reuse the Engine API schema. |
nflaig
left a comment
There was a problem hiding this comment.
Overall looks good to me, this was pretty straight forward to implement ChainSafe/lodestar#8772 for us.
types/primitive.yaml
Outdated
| ClientCode: | ||
| type: string | ||
| description: "A two-character client code that uniquely maps to a client name as defined in the [Engine API](https://github.com/ethereum/execution-apis/blob/main/src/engine/identification.md#clientcode)." | ||
| enum: [BU, EJ, EG, GE, GR, LH, LS, NM, NB, TE, TK, PM, RH] |
There was a problem hiding this comment.
might be worth to specify what to return if client code is unknown, we have a non-spec value of XX for this
| properties: | ||
| code: | ||
| $ref: './primitive.yaml#/ClientCode' | ||
| name: |
There was a problem hiding this comment.
might be worth to specify what name to return if information is missing, eg. if we could not retrieve the EL client info yet, I am just returning Unknown right now
|
I've added the following changes just now:
I left the conversations regarding the "unknown" behavior unresolved for now in case anyone else wants to chime in on that. |
|
LGTM, happy with "unknown" to signal an EL that is unresponsive or doesn't support the version endpoint |
|
I think using "magic strings" like "Unknown", "XX" and empty strings could be problematic because it forces the consumer to know and handle them (they could consider them valid values since non standard). I'd rather prefer making them nullable or optional. And the get method response: TLDR;
|
generally we don't want that because the response can't be ssz encoded if a field is nullable or optional but for this endpoint it doesn't matter, I would be fine with allowing there is one example for this Lines 49 to 52 in f49e171 it doesn't use
this api should never return an error, I don't see why the CL wouldn't be able to return it's own client info |
|
Thanks for the clarifications. for objects. By error I meant the already defined 500 error could be enough (if the API isn't able to respond for any reason) |
|
generally the field would just not be marked required, so if its null its not included in the output... |
not required would be the normal in the beacon-api - we generally don't pass back 'null' fields, i'd far prefer a field thats not required and just dont pass back the field rather than having 'null' or having strings like unknown, but if it was for execution specifically i'd probably be less worried about saying 'unknown' if a non required field is an issue for some reason. |
|
I am also fine with making fields optional if that's what everyone prefers to do |
Adds a
/eth/v2/node/versionendpoint for structured version information that also includes information about the attached EL client.Changes:
/eth/v1/node/versionendpointThe main motivation for this change is for validator clients to have access to information about the EL client attached to a beacon node. Right now, validator clients cannot see "past" the beacon node easily and are therefore blind to what kind of EL client is validating the execution payload. By exposing this information, multi-node validator clients can make safer decisions, e.g. they can require 3 different EL clients to declare the execution payload as valid before they attest to its validity.