Return metadata fields for relevant endpoints

[dB2510]

As per the latest beacon-APIs spec, some endpoints contain execution_optimistic and dependent_root fields as part of their response object. This field was added as part of this PR.

As of now go-eth2-client doesn't support these fields in some of the relevant endpoints.

• At obol while integrating charon to lodestar VC, we found out that on startup while querying for duties it expects these fields to be present.
• The dependent_root field is required to detect reorgs where all validator clients compare the dependent_root returned from getAttesterDuties beacon API response with the dependent roots received from SSE events for head topic. If both of these are different they call getAttesterDuties again.
• Lighthouse VC also expects this field to be present specially in GET /eth/v1/beacon/blocks/{block_id}/root for signing sync committee messages.

In order to support all validator clients and to conform with latest spec, we are suggesting to return these fields as part of response of all the relevant endpoints.

For more details:

• https://github.com/ethereum/consensus-specs/blob/dev/sync/optimistic.md#ethereum-beacon-apis
• Description of getAttesterDuties: https://ethereum.github.io/beacon-APIs/#/Validator/getAttesterDuties

[pk910]

I'd like to give that some priority as the metadata fields are a important part of these endpoints.
The current implementation is not really usable when depending on these fields.

[Willyham]

We also require these fields for correct operation. If we can discuss a plan for the best way to handle these then we can probably contribute time to implement the work.

Reading the data from the server is trivial, but the current interface for clients does not provide any way to surface these fields. Listing possible solutions from the top of my head, let's use the API for SignedBeaconBlock as an example:

SignedBeaconBlock(ctx context.Context, blockID string) (*spec.VersionedSignedBeaconBlock, error)

Option 1.
Return a Metadata struct as a 3rd param from each endpoint:

type Metadata struct {
  ExecutionOptimistic bool
  Finalized bool
}

SignedBeaconBlock(ctx context.Context, blockID string) (*spec.VersionedSignedBeaconBlock, spec.Metadata, error)

Option 2
Change the response type to be a wrapper which contains the metadata:

type VersionedSignedBeaconBlockReponse {
  Metadata Metadata
  Response *spec.VersionedSignedBeaconBlock
}

SignedBeaconBlock(ctx context.Context, blockID string) (*spec.VersionedSignedBeaconBlockReponse, error)

Option 2a
The same as above, but with generics to avoid needing a struct for every endpoint, if you're happy with only supporting >= go 1.18

type Response[T any] struct {
  Metadata Metadata
  Response T
}

SignedBeaconBlock(ctx context.Context, blockID string) (*Response[spec.VersionedSignedBeaconBlock], error)

Option 3
Pick one of the above, but add a new function signature to maintain backwards compatibility:

SignedBeaconBlock(ctx context.Context, blockID string) (*spec.VersionedSignedBeaconBlock, error)
SignedBeaconBlockWithMetadata(ctx context.Context, blockID string) (*Response[spec.VersionedSignedBeaconBlock], error)

Option 4
I really don't like this, but adding it for completeness. It may be possible to cover some % of people's use cases without a breaking change to the api by keeping the 'control' internal to the function:

type Option func(s *Service)

func WithErrorIfNotFinalized(val bool) {
  return func (s *Service) {
    s.ErrorIfNotFinalized = val
  }
}

SignedBeaconBlock(ctx context.Context, blockID string, ...options Option) (*spec.VersionedSignedBeaconBlock, error)

client.SignedBeaconBlock(ctx, "head", WithErrorIfNotFinalized(true))

My preference would be to do option 3 + 2a.

@mcdee would love to hear your thoughts and see if we can align on a direction

[mcdee]

2a is close to my thinking, but we can't have a hard-coded Metadata structure as different endpoints return different metadata. A generic map for metadata isn't very pretty or user-friendly, but is simple. A separate metadata struct for each call makes a bit of a mess of the function signature and requires manual updating of structs,but as we're likely going to have to carry out manual population of the structs anyway I'm inclined to go that way.

3 I'm less of a fan of. I'd be more inclined to take the one-off hit of a change in function signatures rather than have multiple functions that do much the same thing. A more radical option is to have a single call that takes a pointer to an empty struct that is then filled by the relevant call; see #67 (comment) for something similar on the submission side.

I'm still pondering this, but recognize that the metadata is useful and do want to incorporate it somehow.

[Willyham]

Here is a draft PoC which covers 2a but uses a generic map for metadata. Hopefully it gets the ball rolling a bit - I can continue to work on it if you are happy going in this direction.

#77

Let me know how you feel about it.

[mcdee]

#80 has been merged and addresses this item.