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

Keymanager API bugs in validator client #7213

Closed
TobiWo opened this issue Nov 1, 2024 · 4 comments
Closed

Keymanager API bugs in validator client #7213

TobiWo opened this issue Nov 1, 2024 · 4 comments
Labels
meta-bug Issues that identify a bug and require a fix.

Comments

@TobiWo
Copy link

TobiWo commented Nov 1, 2024

Describe the bug

There are several bugs in the keymanager api endpoints. To be more precise:

feerecipient, gas_limt, graffiti

The endpoints:

  • /eth/v1/validator/<pubkey>/feerecipient
  • /eth/v1/validator/<pubkey>/gas_limit

return a 500 if the pubkey is not managed by lodestar. This is the response:

{
    "statusCode": 500,
    "error": "Internal Server Error",
    "message": "Validator pubkey <pubkey> not known"
}

The endpoint:

  • /eth/v1/validator/<pubkey>/graffiti

returns 200 and a correct response even if the pubkey is NOT managed by lodestar. I'm not sure if this is intended behavior as the graffiti is obviously no secret. However, since there are also DELETE and POST methods available, which obviously will only work for locally managed validators, I would assume that also GET only returns a 200 if the pubkey is managed by lodestar. I know that the keymanager api spec does not have a clear definition with regards to that topic but as mentioned I would assume that it only returns the locally managed validator graffitis (as other clients do).

keystores

The endpoint:

  • /eth/v1/keystores

returns the managed keystores even if these are actually managed by a remote signer. So I will receive the same data on /eth/v1/keystores and /eth/v1/remotekeys when validators are managed by a connected remote signer.

Expected behavior

endpoint scenario status code possible response
/eth/v1/validator/<pubkey>/feerecipient pubkey not managed by lodestar 404 {"message": "Could not find validator"}
/eth/v1/validator/<pubkey>/gas_limit pubkey not managed by lodestar 404 {"message": "Could not find validator"}
/eth/v1/validator/<pubkey>/graffiti pubkey not managed by lodestar 404 {"message": "Could not find validator"}
/eth/v1/keystores keystores are managed by remote signer 200 {"data": []}

Steps to reproduce

Kurtosis devnet definition to reproduce:

---
participants:
  - el_type: "reth"
    el_image: "ghcr.io/paradigmxyz/reth:v1.1.0"
    cl_type: "grandine"
    cl_image: "sifrai/grandine:0.4.1"
  - el_type: "geth"
    el_image: "ethereum/client-go:v1.14.11"
    cl_type: "lighthouse"
    cl_image: "sigp/lighthouse:v5.3.0"
  - el_type: "geth"
    el_image: "ethereum/client-go:v1.14.11"
    cl_type: "teku"
    cl_image: "consensys/teku:24.10.2"
    use_separate_vc: true
  - el_type: "besu"
    el_image: "hyperledger/besu:24.10.0"
    cl_type: "lodestar"
    cl_image: "chainsafe/lodestar:v1.22.0"
    validator_count: 4
  - el_type: "nethermind"
    el_image: "nethermind/nethermind:1.29.1"
    cl_type: "teku"
    cl_image: "consensys/teku:24.10.2"
    use_separate_vc: true
keymanager_enabled: true
...

Start devnet:
kurtosis run --image-download always --enclave lodestar-bug-devnet github.com/ethpandaops/ethereum-package --args-file <path-to-above-yaml>

Execute above mentioned rest calls (see bug description).

Additional context

No response

Operating system

Linux

Lodestar version or commit hash

v1.22.0

@TobiWo TobiWo added the meta-bug Issues that identify a bug and require a fix. label Nov 1, 2024
@nflaig
Copy link
Member

nflaig commented Nov 1, 2024

Thanks for reporting!

return a 500 if the pubkey is not managed by lodestar. This is the response:

I agree, we should throw a 404 here instead.

returns 200 and a correct response even if the pubkey is NOT managed by lodestar.

that's incorrect, we should also return 404 in that case, good catch

returns the managed keystores even if these are actually managed by a remote signer.

From a spec perspective I would definitely agree, I wonder if people actually rely on this faulty behavior since it works like this for ~3 years. Although it's a pretty rare that people have local + remote keys on the same validator client. Did you check how other clients handle this?

@nflaig
Copy link
Member

nflaig commented Nov 1, 2024

However, since there are also DELETE and POST methods available, which obviously will only work for locally managed validators

Regarding this, why would you only be able to do that for local pubkeys? The validator client is the one that has to pass the graffiti to the beacon node, the web3signer can't configure it since it only signs over the data that the VC provides it.

Same for fee recipient and gas limit

@TobiWo
Copy link
Author

TobiWo commented Nov 2, 2024

Thanks for reporting!

You guys are welcome 🙂 .

Although it's a pretty rare that people have local + remote keys on the same validator client. Did you check how other clients handle this?

I think you misunderstood me. I just tested the behavior for keys which either are managed locally or managed by the remote signer. I did not test how it works if you have keys managed locally and other keys managed by the remote signer. I also think that this is such a small niche that it does not matter. Just from a pure logical standpoint, as the names suggest, the keystores endpoint should return all locally managed keys and remotekeys should return all remotely managed keys 🙂 .

Regarding this, why would you only be able to do that for local pubkeys?

My fault. I think I may have phrased that poorly. locally managed validators should have been own managed keys, either managed locally or via a remote signer. So I absolutely agree with your statement 😉 .

@nflaig
Copy link
Member

nflaig commented Nov 3, 2024

Resolved by #7214 and #7215

@nflaig nflaig closed this as completed Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-bug Issues that identify a bug and require a fix.
Projects
None yet
Development

No branches or pull requests

2 participants