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

fix: handle encoded commas in URL query parameters #6133

Closed
wants to merge 1 commit into from

Conversation

eth2353
Copy link
Contributor

@eth2353 eth2353 commented Nov 27, 2023

Motivation

The Teku validator client is unable to fetch validator statuses from a Lodestar beacon node if more than 1 validator status is requested due to the way they request the data. Lodestar should be able to serve Teku's requests.

Description

The issue is caused by the differing ways the validator clients encode the query parameter values.

Lodestar VC request:

GET /eth/v1/beacon/states/head/validators?id=0x978e96b6b6b474f3f91b705a311906b14a5e2893d15e01f5a0ff210393ace5e0abc8488e844dab00f430364ba4c19b1a&id=0x99f013b0314981b0083e494bdd2989304f9fc9dfd2b0c23b40429d817b02acc78f0b4328e45e745a5c6cc48e8f0e9930 HTTP/1.1

Teku VC request:

GET /eth/v1/beacon/states/head/validators?id=0x978e96b6b6b474f3f91b705a311906b14a5e2893d15e01f5a0ff210393ace5e0abc8488e844dab00f430364ba4c19b1a%2C0x99f013b0314981b0083e494bdd2989304f9fc9dfd2b0c23b40429d817b02acc78f0b4328e45e745a5c6cc48e8f0e9930 HTTP/1.1
-> the comma is URI-encoded as %2C . Lodestar then returns an empty array of validator statuses, resulting in Teku VC error messages (and the Teku VC therefore not starting validator duties for the affected validators):

19:14:03.083 WARN  - Unable to retrieve status for validator 978e96b.
19:14:03.083 WARN  - Unable to retrieve status for validator 99f013b.

I did not test other VC clients but it is possible others do the same.
The Beacon API Swagger seems to result in the comma unencoded in the request.

After reading some discussions on this topic, I am still unsure about which way should be considered technically correct and therefore decided to try supporting both ways on the Lodestar side.

This PR implements support for URI-encoded query parameter values, like the ones Teku VC sends. It is backwards compatible - it supports the comma encoded but also unencoded.

@eth2353 eth2353 requested a review from a team as a code owner November 27, 2023 10:43
@CLAassistant
Copy link

CLAassistant commented Nov 27, 2023

CLA assistant check
All committers have signed the CLA.

@nflaig
Copy link
Member

nflaig commented Nov 27, 2023

Thanks for looking into this. There has been discussion on this topic in a previous issue #5403. As per RFC 3986, handling %2C as a delimiter is not correct, see #5403 (comment) for more detailed explanation.

@eth2353
Copy link
Contributor Author

eth2353 commented Nov 27, 2023

Thanks for your quick response, I thought I had checked everywhere but I did somehow miss this issue. I was not able to find any more follow-up discussion in the R&D discord. I've opened Consensys/teku#7757 in Teku's repository which fixes it from the other side.

@eth2353 eth2353 closed this Nov 27, 2023
@nflaig
Copy link
Member

nflaig commented Nov 27, 2023

Thanks for opening the issue on Teku side, let's continue the discussion there if required.

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