use header based selector with slot fallback #8522
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements support for the 'Eth-Consensus-Version' header in Teku's beacon node API, improving version-aware request handling and type definition selection.
- Added
headerBasedSelectorWithSlotFallbackinMilestoneDependentTypesUtil.javafor flexible header-based selection - Updated V2 API endpoints (
PostBlindedBlockV2,PostBlockV2) to require the 'Eth-Consensus-Version' header - Modified V1 API endpoints (
PostBlindedBlock,PostBlock) to use header-based selection with slot fallback - Enhanced error handling and messaging for missing or invalid 'Eth-Consensus-Version' headers
- Improved test coverage in
MilestoneDependentTypesUtilTestfor different SpecMilestones and header scenarios
9 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
rolfyone
reviewed
Aug 22, 2024
Comment on lines
+122
to
+123
| // SignedBlockContents | ||
| .or(() -> getSlot(json, "signed_block", "message", "slot")); |
There was a problem hiding this comment.
is block contents still a thing? I thought that went away?
There was a problem hiding this comment.
Yep, we added them since Deneb to handle block and blobs. thought we could remove this use case but turned out necessary when we send Deneb block contents to the post block V1 apis, they might retrieve the slot in order to get the correct schema
rolfyone
approved these changes
Aug 22, 2024
mehdi-aouadi
force-pushed
the
7720-header-selector
branch
from
0dbd265 to
e8ea327
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
Use the
Eth-Consensus-Versionheader when available to determine the request body type definition and fallback to the slot selector when the header is not required and not present:Eth-Consensus-Versionrequired: In that case we don't fallback to the slot selector since the request should fail when the consensus version header isn't provided or a bad value is used, we use theheaderBasedSelectorwithout fallback to the slot. This is the case for the V2 APIs: /eth/v2/beacon/pool/attestations, /eth/v2/beacon/pool/attester_slashings, /eth/v2/beacon/blinded_blocks and /eth/v2/beacon/blocksEth-Consensus-Versionoptional: In taht case, we try to get the type definition using the header otherwise we fall back to the slot based selector if the header is not present, we use theheaderBasedSelectorWithSlotFallback. This is the case for the V1 APIs: /eth/v1/beacon/blinded_blocks and /eth/v1/beacon/blocksPS: This only covers the json content type use case. For the ssz use case, we always fall back to the slot selector even when the header is required but not provided which is not ideal IMO. I thought we should maybe be more strict about that and make the request fail when the header is required but not provided for the ssz content type
Fixed Issue(s)
fixes #7720
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog