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 encoding of comma character in query parameters #7757

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

eth2353
Copy link
Contributor

@eth2353 eth2353 commented Nov 27, 2023

PR Description

The Teku VC encodes the comma , character in its request to the /validators endpoint, resulting in a request like this:

GET /eth/v1/beacon/states/head/validators?id=0x978e96b6b6b474f3f91b705a311906b14a5e2893d15e01f5a0ff210393ace5e0abc8488e844dab00f430364ba4c19b1a%2C0x99f013b0314981b0083e494bdd2989304f9fc9dfd2b0c23b40429d817b02acc78f0b4328e45e745a5c6cc48e8f0e9930 HTTP/1.1

This is incompatible with Lodestar's BN, which handles it as one long ID containing the %2C string, resulting in an empty array of statuses being returned. This in turn results in the Teku VC failing to get the status for its validators (and subsequently not performing validator duties for them):

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

This was discussed in ChainSafe/lodestar#5403 . The conclusion there was the comma character should not be encoded in this case.

The Teku BN API can handle both cases, regardless of whether the comma character is encoded or not, so this change is completely backwards compatible.

Looking forward to hear your thoughts on this.

Fixed Issue(s)

I have not found an issue for this in this repository.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@eth2353
Copy link
Contributor Author

eth2353 commented Nov 27, 2023

This kind of incompatibility will likely disappear with ethereum/beacon-APIs#367 but I think it still deserves to at least be considered to be fixed.

@CLAassistant
Copy link

CLAassistant commented Nov 27, 2023

CLA assistant check
All committers have signed the CLA.

@rolfyone
Copy link
Contributor

Hey thanks for raising this, it makes complete sense.

We do have a change coming to the validators endpoint where it becomes a POST, but having said that, we're yet to change to the post, so happy to merge this one :)

@rolfyone rolfyone enabled auto-merge (squash) November 29, 2023 03:09
Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@rolfyone rolfyone merged commit 1f7dac4 into Consensys:master Nov 29, 2023
13 of 15 checks passed
@eth2353 eth2353 deleted the query_param_encoding branch December 18, 2023 15:55
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