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

Prysm VC more robust content-type header handling #14093

Closed
nflaig opened this issue Jun 8, 2024 · 2 comments
Closed

Prysm VC more robust content-type header handling #14093

nflaig opened this issue Jun 8, 2024 · 2 comments
Assignees
Labels
API Api related tasks Bug Something isn't working

Comments

@nflaig
Copy link

nflaig commented Jun 8, 2024

Describe the bug

Was looking into ChainSafe/lodestar#6633 compatibility issue of Lodestar BN <> Prsym VC.

The error on the Prysm side

time="2024-06-08 20:13:34" level=info msg="Syncing with beacon node to align on chain genesis info" prefix=client
time="2024-06-08 20:13:34" level=warning msg="Could not determine if beacon chain started" error="could not receive ChainStart from stream: failed to get genesis data: genesis data is nil: could not connect" prefix=client

But the response from Lodestar looks correct

time="2024-06-08T20:13:44Z" level=info msg="REQUEST #2: GET /eth/v1/beacon/genesis" length=0 type=json
time="2024-06-08T20:13:44Z" level=info msg="RESPONSE #2: GET /eth/v1/beacon/genesis" length=169 status=200 type=json
{
  "data": {
    "genesis_fork_version": "0x10000038",
    "genesis_time": "1717877689",
    "genesis_validators_root": "0x70498f18a6f07107a2a05d0d36c7cbd93b3e439023cbef21a6681eebee194526"
  }
}

I believe the reason why the data is nil is due to the way the content-type header is checked

if httpResp.Header.Get("Content-Type") != api.JsonMediaType {

it compares the whole header against the media type but in reality, the conten type header can also include the charset and boundry (see Content-Type#directives).

Lodestar returns the following header

content-type: application/json; charset=utf-8

which is a valid content-type header format, on Prysm side you probably wanna apply something like

strings.SplitN(contentType, ";", 2)[0]

Has this worked before in a previous version?

No response

🔬 Minimal Reproduction

Kurtosis config

participants:
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: lodestar
    cl_image: chainsafe/lodestar:latest
    vc_type: prysm
    vc_image: gcr.io/prysmaticlabs/prysm/validator:latest
    count: 2
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: prysm
    cl_image: gcr.io/prysmaticlabs/prysm/beacon-chain:latest
    vc_type: lodestar
    vc_image: chainsafe/lodestar:latest

Error

No response

Platform(s)

Linux (x86)

What version of Prysm are you running? (Which release)

v5.0.3

Anything else relevant (validator index / public key)?

Related issue ChainSafe/lodestar#6633

@nflaig nflaig added the Bug Something isn't working label Jun 8, 2024
@nflaig nflaig changed the title Prysm VC more robust content-type header parsing Prysm VC more robust content-type header handling Jun 8, 2024
@james-prysm james-prysm added the API Api related tasks label Jun 10, 2024
@james-prysm james-prysm self-assigned this Jun 10, 2024
@james-prysm
Copy link
Contributor

Thanks for the report will fix!

@nflaig
Copy link
Author

nflaig commented Jun 11, 2024

All Lodestar <> Prysm issues seem to be resolved 🎉
image

Running the following config

participants:
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: lodestar
    cl_image: nflaig/lodestar:ignore-empty-statuses
    vc_type: prysm
    vc_image: ethpandaops/prysm-validator:develop-dfe31c9
    count: 2
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: prysm
    cl_image: ethpandaops/prysm-beacon-chain:develop-dfe31c9
    vc_type: lodestar
    vc_image: nflaig/lodestar:ignore-empty-statuses
    vc_extra_params:
      - --http.requestWireFormat=ssz
    count: 2

I can also confirm that #14075 works as expected, we can enable ssz-only mode on Lodestar with Prysm without any errors (as it falls back to JSON as expected now).

Just a quick side note, I noticed another issue that Prysm VC sends an empty statuses array when calling POST /eth/v1/beacon/states/head/validators. I would argue this is inline with the spec and I improved the handling on Lodestar side (ChainSafe/lodestar#6876) but previously this would just result in returning no validators (irrespective of ids passed).

@nflaig nflaig closed this as completed Jun 11, 2024
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants