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

feat: add response headers to produceBlockV3 #6228

Merged
merged 4 commits into from
Dec 23, 2023

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Dec 22, 2023

Motivation

Some tools (eg. blockdreamer) rely on Eth-Consenus-Version header from produceBlockV3 before further processing.

Description

This is a temporary solution to add the 4 headers (Eth-Consensus-Version, Eth-Execution-Payload-Blinded, Eth-Execution-Payload-Value, Eth-Consensus-Block-Value) for produceBlockV3 according to the spec before a more permanent solution of returning api response headers is introduced.

Closes #6202

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Merging #6228 (c2dda6d) into unstable (ae04197) will increase coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #6228      +/-   ##
============================================
+ Coverage     80.81%   80.83%   +0.02%     
============================================
  Files           185      185              
  Lines         17964    17986      +22     
  Branches       1082     1082              
============================================
+ Hits          14517    14539      +22     
  Misses         3421     3421              
  Partials         26       26              

@ensi321 ensi321 marked this pull request as ready for review December 22, 2023 08:23
@ensi321 ensi321 requested a review from a team as a code owner December 22, 2023 08:23
...serverRoutes.produceBlockV3,
handler: async (req, res) => {
const response = await api.produceBlockV3(...reqSerializers.produceBlockV3.parseReq(req));
void res.header("Eth-Consensus-Version", response.version);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want set other headers here as well? My guess is that most should get those from the json body anyways though

Copy link
Contributor Author

@ensi321 ensi321 Dec 22, 2023

Choose a reason for hiding this comment

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

Do we want set other headers here as well? My guess is that most should get those from the json body anyways though

I was trying to ask sproul if he needs the other headers but it doesn't seem like he needs them.

I guess it's easy enough to add the other headers. It doesn't hurt to add them now so might as well

Copy link
Member

Choose a reason for hiding this comment

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

One optimization could be that you just look at the block value header and not even parse the body, might be what vouch is doing if it requests blocks from multiple bns

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

lgtm

@nflaig nflaig changed the title feat: add Eth-Consensus-Version header to produceBlockV3 feat: add response headers to produceBlockV3 Dec 22, 2023
@nflaig nflaig merged commit 61cf1a8 into ChainSafe:unstable Dec 23, 2023
17 of 18 checks passed
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.14.0 🎉

ensi321 added a commit to ensi321/lodestar that referenced this pull request Jan 22, 2024
* Add `Eth-Consensus-Version` to produceBlockV3

* Lint

* Lint

* Add header
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.

BlockV3 API does not return Eth-Consensus-Version
3 participants