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

publish block fix for validator interoperability with lighthouse beacon #3376

Merged
merged 4 commits into from
Oct 30, 2021

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented Oct 18, 2021

Motivation
This is to make the lodestar validator interop with lighthouse beacon

Description
The lodestar validator was throwing errors on the produceBlock api response as documented in isssue: #3370 .

Reason was that the lodestar validator assumes the version information (phase0, altair) availability in response which was not being provided by lighthouse beacon as its not the part of the spec. This PR adds the ability to extract the version from the slot to get the version.

making WithVersion back compatible with pre altair v1/produceBlock call

The publishBlock api is now being properly consumed and the validator is able to produce blocks.
image

Closes #issue_number
#3370

Steps to test or reproduce

@CLAassistant
Copy link

CLAassistant commented Oct 18, 2021

CLA assistant check
All committers have signed the CLA.

@codeclimate
Copy link

codeclimate bot commented Oct 18, 2021

Code Climate has analyzed commit 81d5080 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #3376 (81d5080) into master (861f6e4) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3376      +/-   ##
==========================================
- Coverage   38.37%   38.36%   -0.01%     
==========================================
  Files         303      303              
  Lines        7735     7738       +3     
  Branches     1155     1157       +2     
==========================================
+ Hits         2968     2969       +1     
  Misses       4628     4628              
- Partials      139      141       +2     

@dapplion
Copy link
Contributor

@g11tech It is part of the REST API spec! Can you check with Lighthouse devs if they are not adding this info? If not, you should raise an issue on their side.

@dapplion
Copy link
Contributor

@g11tech According to their source code Lighthouse is adding the fork version to the response:

                            let fork_name = block.fork_name(&chain.spec).ok();
                            fork_versioned_response(endpoint_version, fork_name, block)
                                .map(|res| warp::reply::json(&res).into_response())

https://github.com/sigp/lighthouse/blob/fff01b24ddedcd54486e374460855ca20d3dd232/beacon_node/http_api/src/lib.rs?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L1019

@g11tech
Copy link
Contributor Author

g11tech commented Oct 18, 2021

@g11tech According to their source code Lighthouse is adding the fork version to the response:

                            let fork_name = block.fork_name(&chain.spec).ok();
                            fork_versioned_response(endpoint_version, fork_name, block)
                                .map(|res| warp::reply::json(&res).into_response())

https://github.com/sigp/lighthouse/blob/fff01b24ddedcd54486e374460855ca20d3dd232/beacon_node/http_api/src/lib.rs?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L1019

@dapplion actually on the testnet, when slots start from the genesis, till altair fork epoch, v1/produceBlock is being called instead of v2/productBlock by our validator:
https://github.com/ChainSafe/lodestar/blob/master/packages/validator/src/services/block.ts#L76:L80
and v1 doesn't have the version type.
a very simple approach would be that in WithVersion, getType(version) can be replaced by getType(version||'phase0')

@dapplion
Copy link
Contributor

dapplion commented Oct 28, 2021

Oh the root of the problem is that our API is not following the spec. The route produceBlock does not include version. https://github.com/ethereum/beacon-APIs/blob/295ce41a45e297aa21bea5d1695b3060066320e0/apis/validator/block.yaml?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L39

I proposed this solution, please take a look and adopt it in your PR if you agree master...linterop

@g11tech
Copy link
Contributor Author

g11tech commented Oct 28, 2021

Oh the root of the problem is that our API is not following the spec. The route produceBlock does not include version. https://github.com/ethereum/beacon-APIs/blob/295ce41a45e297aa21bea5d1695b3060066320e0/apis/validator/block.yaml?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L39

I proposed this solution, please take a look and adopt it in your PR if you agree master...linterop

yes, separate serialization for produceBlock makes sense. will incorporate the suggestion and bring it to a working stage! 👍

@g11tech
Copy link
Contributor Author

g11tech commented Oct 29, 2021

@dapplion now the validator is nicely publishing blocks on the lighthouse beacon!

image

Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

🚀🚀🙏🙏🎉🎉

@dapplion dapplion merged commit aafc5d7 into ChainSafe:master Oct 30, 2021
@g11tech g11tech deleted the linterop branch October 30, 2021 10:24
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.

None yet

3 participants