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

Add getHeader SSZ support #734

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from
Open

Add getHeader SSZ support #734

wants to merge 30 commits into from

Conversation

jtraglia
Copy link
Collaborator

@jtraglia jtraglia commented Feb 5, 2025

πŸ“ Summary

This PR adds SSZ support for getHeader endpoint. Please see:

βœ… I have run these commands

  • make lint
  • make test-race
  • go mod tidy

@0xTylerHolmes
Copy link
Contributor

0xTylerHolmes commented Feb 5, 2025

I think they are fine, but two things that caught my attention:

  1. Reusing a request object (it is fine if body == nil)
  2. Double close scenarios (io.Closer specifies that calls to Close after first call is considered undefined; though I think it is fine here)

First thought that addresses both of these and simplifies things a bit is if we define a doRequest function inside get_header that we can use to handle both the SSZ and JSON request attempts:

doRequest := func(accept string) (*http.Response, error) {
                req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil)
                if err != nil {
                    return nil, err
                }
                // <insert request prep>
                req.Header.Set("Accept", accept)
                return m.httpClientGetHeader.Do(req)
            }

Then we can do something like:

            // ssz attempt
            resp, err := doRequest("application/octet-stream")
            if err != nil {
                …
                return
            }
            // json if relay does not support ssz
            if resp.StatusCode == http.StatusNotAcceptable {
                resp.Body.Close()
                logRelay.Debug("SSZ not accepted, trying JSON")
                resp, err = doRequest("application/json")
                if err != nil {
                    …
		    return
                }
            }
            defer resp.Body.Close()
            // continue rest of execution
            ...

edit: get_header should probably use a context that we pass to the NewRequestWithContext so we can handle timeouts.

}

// Check if the relay supports SSZ requests
if resp.StatusCode != http.StatusNotAcceptable {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this approach works I am not sure about the feasibility in practice due to time constraints, I think it would be best to use q-weighted Accept headers as noted here. To handle the response body, you can look at the Content-Type header sent by server.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting. So this can/should be done in a single request. I'll just forward whatever the client sends & handle the response with Content-Type like you suggest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nflaig if you have time to review this again, I would really appreciate it πŸ™

}

// Get the response's content type
respContentType := resp.Header.Get("Content-Type")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do all relays properly set this header, otherwise might be best to assume JSON if header is missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Defaulting to JSON sounds like a good idea to me. Will make this change.


// Add headers from the request to this request.
// This includes Accept and Eth-Consensus-Version, if provided.
for key, values := range header {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my only worry here would be that relays don't properly handle q-weighted Accept headers, which is fine as long as they default to JSON in that case, if they throw an error it would be problematic though

}

// ParseAcceptHeader parses and sorts the Accept header by q-values.
func ParseAcceptHeader(header string) []AcceptEntry {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you need some test cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've incorporated these into this PR. But with two changes. If there is no accept header or the accept header isn't in the supported media types, we will default to JSON.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no accept header or the accept header isn't in the supported media types, we will default to JSON.

we do the same although would return 406 if client only accepts media types we don't support, it's handled here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes. I like that idea. Will do the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants