ChainSafe / lodestar

🌟 TypeScript Implementation of Ethereum Consensus
https://lodestar.chainsafe.io
Apache License 2.0
1.2k stars 300 forks source link

Grandine CL < > Lodestar VC incompatibility #6637

Closed barnabasbusa closed 7 months ago

barnabasbusa commented 8 months ago

Describe the bug

We are in the process to test cross beacon <> validator client compatibility, and found a bug when testing Grandine CL with lodestar VC.

Lodestar reports:

Apr-05 11:27:04.125[]                error: Error proposing block slot=1, validator=0xa158…501f - Failed to produce block: validator.produceBlockV3 - Bad Request: invalid query string: unknown field `fee_recipient`, expected one of `randao_reveal`, `graffiti`, `skip_randao_verification` - Failed to produce block
Error: Failed to produce block: validator.produceBlockV3 - Bad Request: invalid query string: unknown field `fee_recipient`, expected one of `randao_reveal`, `graffiti`, `skip_randao_verification` - Failed to produce block
    at Function.assert (file:///usr/app/packages/api/src/utils/client/httpClient.ts:44:13)
    at BlockProposingService.produceBlockWrapper (file:///usr/app/packages/validator/src/services/block.ts:212:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at BlockProposingService.createAndPublishBlock (file:///usr/app/packages/validator/src/services/block.ts:144:29)
    at async Promise.all (index 0)

Grandine reports:

nothing out of the ordinary

Snooper between cl <> vc reports:

nothing out of the ordinary

cc: @pk910

Expected behavior

I would expect all client combinations would work.

Steps to reproduce

config.yaml:

participants:
  - cl_type: grandine
    vc_type: lodestar
additional_services:
  - dora
snooper_enabled: true
global_log_level: debug

kurtosis run github.com/kurtosis-tech/ethereum-package --args-file config.yaml

Additional context

Current BN <> VC Compatibility list tracker

Screenshot 2024-04-05 at 14 08 53

Operating system

Linux

Lodestar version or commit hash

Version: v1.17.0/898cd90

barnabasbusa commented 8 months ago

Cross reference of: https://github.com/grandinetech/grandine/issues/24

sauliusgrigaitis commented 8 months ago

Any chance for Lodestar to not send this field by default? (Maybe you can send it if CLI flag is passed)

nflaig commented 8 months ago

Any chance for Lodestar to not send this field by default? (Maybe you can send it if CLI flag is passed)

This is definitely an issue on our end, we were sending this query param always with a boolean value, e.g. by default skip_randao_verification=false. We have fixed this recently https://github.com/ChainSafe/lodestar/pull/6576 but it's not yet released.

sauliusgrigaitis commented 8 months ago

@nflaig I just noticed that @barnabasbusa mixed errors Discord.

The problem is actually fee_recipient field:

Apr-05 11:27:04.125[]                error: Error proposing block slot=1, validator=0xa158…501f - Failed to produce block: validator.produceBlockV3 - Bad Request: invalid query string: unknown field `fee_recipient`, expected one of `randao_reveal`, `graffiti`, `skip_randao_verification` - Failed to produce block
Error: Failed to produce block: validator.produceBlockV3 - Bad Request: invalid query string: unknown field `fee_recipient`, expected one of `randao_reveal`, `graffiti`, `skip_randao_verification` - Failed to produce block
    at Function.assert (file:///usr/app/packages/api/src/utils/client/httpClient.ts:44:13)
    at BlockProposingService.produceBlockWrapper (file:///usr/app/packages/validator/src/services/block.ts:212:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at BlockProposingService.createAndPublishBlock (file:///usr/app/packages/validator/src/services/block.ts:144:29)
    at async Promise.all (index 0)
barnabasbusa commented 8 months ago

@nflaig indeed, I mixed up the issues with nimbus and grandine. Please see the correct error message above.

@sauliusgrigaitis good catch!

nflaig commented 8 months ago

I am not sure we can resolve this on the Lodestar side as we have a bunch of query params that are not part of the spec but Lodestar itself uses.

https://github.com/ChainSafe/lodestar/blob/898cd903bf9d65836ac06e43740efcc5404fe615/packages/api/src/beacon/routes/validator.ts#L489-L494

It seems like all other clients just ignore extraneous query params and we should probably avoid implementing client specific workarounds.

But let's ask @g11tech who added most / all of those extra params.

sauliusgrigaitis commented 8 months ago

@nflaig @g11tech is it the only request that you send unspec'ed parameters?

nflaig commented 8 months ago

@nflaig @g11tech is it the only request that you send unspec'ed parameters?

Yes, I think this is the only api. Also since we allow to produce a blinded local block, we send execution_payload_source as part of the response body, this has been proposed in https://github.com/ethereum/beacon-APIs/issues/387 but did not make it into block v3 api.

barnabasbusa commented 8 months ago

Trying again with grandine's latest image (which ignores extra fields) ethpandaops/grandine:ignore-extra-fields-c28d828

Apr-05 14:33:42.048[]                error: Error proposing block slot=72, validator=0x8419…5932 - Failed to produce block: validator.produceBlockV3 - Internal Server Error: {
  "id": 1,
  "jsonrpc": "2.0",
  "error": {
    "code": -32603,
    "message": "Error processing response: encountered hyper error: hyper::Error(IncompleteMessage)"
  }
} - Failed to produce block
Error: Failed to produce block: validator.produceBlockV3 - Internal Server Error: {
  "id": 1,
  "jsonrpc": "2.0",
  "error": {
    "code": -32603,
    "message": "Error processing response: encountered hyper error: hyper::Error(IncompleteMessage)"
  }
} - Failed to produce block
    at Function.assert (file:///usr/app/packages/api/src/utils/client/httpClient.ts:44:13)
    at BlockProposingService.produceBlockWrapper (file:///usr/app/packages/validator/src/services/block.ts:212:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at BlockProposingService.createAndPublishBlock (file:///usr/app/packages/validator/src/services/block.ts:144:29)
    at async Promise.all (index 0)
nflaig commented 8 months ago

Trying again with grandine's latest image (which ignores extra fields) ethpandaops/grandine:ignore-extra-fields-c28d828

Looks like an internal error in Grandine

sauliusgrigaitis commented 8 months ago

@barnabasbusa @nflaig It's skip_randao_verification=false now. @barnabasbusa maybe you can build Lodestar image with tha t fix https://github.com/ChainSafe/lodestar/pull/6576 ?

nflaig commented 8 months ago

@barnabasbusa @nflaig It's skip_randao_verification=false now. @barnabasbusa maybe you can build Lodestar image with tha t fix #6576 ?

You can just use chainsafe/lodestar:next which is build from our unstable branch on every commit

g11tech commented 7 months ago

i would request grandine @sauliusgrigaitis to ignore the non spec params, they are essentially there because we need them for one purpose or another.

their defaults are spec complaint (so they assume values in accordance with spec) and responses are modified only if they were requested by these non spec aware valdiators/middlewares (DVT)

and extra response flags carry the information if they are applied as well with again defaults of those flags in spec complaint manner which again a non aware validator software for e.g. can safely ignore.

pk910 commented 7 months ago

This is fixed in latest grandine develop version :) Grandine is now ignoring these additional parameters, so probably nothing to do from lodestar side.

sauliusgrigaitis commented 7 months ago

We ignore not spec'ed parameters in in particular places to achieve compatibility. However, we will probably ignore it all requests later. So this can be closed I think.

nflaig commented 7 months ago

We ignore not spec'ed parameters in in particular places to achieve compatibility. However, we will probably ignore it all requests later. So this can be closed I think.

Great, thanks @sauliusgrigaitis, I also think this is the best behavior as it gives clients the opportunity to experiment with new configurations even if those are not part of the spec yet.

Thanks for retesting @pk910 :pray: