-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
fix: set Eth-Consensus-Version header in publish block requests #6593
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6593 +/- ##
=========================================
Coverage 61.49% 61.49%
=========================================
Files 556 556
Lines 58895 58903 +8
Branches 1856 1857 +1
=========================================
+ Hits 36216 36222 +6
- Misses 22638 22640 +2
Partials 41 41 |
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to make sure to test blinded block publish flow as well, can be easily done by adding --blindedLocal
flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, giving this a try locally as spec tests don't cover headers yet
@@ -335,6 +341,7 @@ export function getReqSerializers(config: ChainForkConfig): ReqSerializers<Api, | |||
writeReq: (item, {broadcastValidation} = {}) => ({ | |||
body: AllForksSignedBlockOrContents.toJson(item), | |||
query: {broadcast_validation: broadcastValidation}, | |||
headers: {"Eth-Consensus-Version": config.getForkName(extractSlot(item))}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would probably just inline the logic here, avoids jumping in multiple functions if you wanna review the code but doesn't matter much for this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 This PR is included in v1.18.0 🎉 |
Motivation
Follow the beacon-APIs specs.
Note that submitBlindedBlock reuses publishBlindedBlock serializers.
Description
Properly sets
Eth-Consensus-Version
when calling relevant beacon-APIs endpoints. This is a short term fix. The proper solution will ship via a later refactoring.Fixes #6580