getStateValidators does not dedupe validators

[nflaig]

Describe the bug

Not sure if this should be considered a bug but I think it is better that we dedupe the result of getStateValidators if the same validator index/pubkey is provided multiple times.

Current behavior

Returns the same validator multiple times if index or pubkey is provided multiple times.

See implementation

lodestar/packages/beacon-node/src/api/impl/beacon/state/index.ts

Line 58 in 3e254d5

async getStateValidators(stateId, filters) {

Expected behavior

Should dedupe the result and only return each unique validator once.

Steps to Reproduce

Start lodestar in dev mode

./lodestar dev  --genesisValidators 8 --startValidators 0..7

send http request to getStateValidators API

curl "http://localhost:9596/eth/v1/beacon/states/head/validators?id=1,1" | jq

be surprised that the validator with index 1 is returned twice

{
  "data": [
    {
      "index": "1",
      "balance": "32000000000",
      "status": "active_ongoing",
      "validator": {
        "pubkey": "0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b",
        "withdrawal_credentials": "0x00ec7ef7780c9d151597924036262dd28dc60e1228f4da6fecf9d402cb3f3594",
        "effective_balance": "32000000000",
        "slashed": false,
        "activation_eligibility_epoch": "0",
        "activation_epoch": "0",
        "exit_epoch": "18446744073709551615",
        "withdrawable_epoch": "18446744073709551615"
      }
    },
    {
      "index": "1",
      "balance": "32000000000",
      "status": "active_ongoing",
      "validator": {
        "pubkey": "0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b",
        "withdrawal_credentials": "0x00ec7ef7780c9d151597924036262dd28dc60e1228f4da6fecf9d402cb3f3594",
        "effective_balance": "32000000000",
        "slashed": false,
        "activation_eligibility_epoch": "0",
        "activation_epoch": "0",
        "exit_epoch": "18446744073709551615",
        "withdrawable_epoch": "18446744073709551615"
      }
    }
  ],
  "execution_optimistic": false
}

[dapplion]

I mean, you requested the validator at index 1 twice, so it's not wrong tor return the validator with index 1 twice. What does the spec says about this?

[nflaig]

@dapplion There is no clear guidance on this in the Beacon API spec itself and neither does the OpenAPI spec details on how to handle duplicate keys in an array query string parameter.

However, the in the Beacon API spec it is at least enforced to have uniqueItems which would fail the example query of the issue during schema validation. But then the question would still be what do we do if the same validator is queries with pubkey and index, the schema validation would not fail in that case.

Regarding, stricter schema validation, I think we can not enforce this at this point as it likely would just cause issues without a major benefit in most cases.

I think it is fine how our API currently behaves, and considering breaking things if we change the behavior we might just wanna keep it as is. The most important thing is that our APIs behave the same across the board to allow users to make assumptions and potentially save some time.

This issue is mostly intended to at least quickly discuss this topic

[dapplion]

If the spec says uniqueItems, then let's obey by it. uniqueItems seems to mean to return 400 if the request contains duplicate items

[nflaig]

@dapplion the spec also says maxItems 30 but nobody (not a single CL client) enforces that as far as I have seen. That's a bit of a pity in my opinion as stricter APIs are usually a good thing to ensure consistency and predictability, even across different implemenations.

Unfortunately, I think it is too late to fully enforce the OpenAPI spec requirements as downstream tooling relies on this lax API validation. For example, rocketpool sends a request like this eth/v1/beacon/states/head/validators?id=<1000 validator pubkeys, comma-separated> and it works with all CL clients.

[dapplion]

Ok, then let's de-duplicate

[wemeetagain]

@nflaig is this still useful?

[nflaig]

yeah, I think we should dedupe the response although this is low prio