-
Notifications
You must be signed in to change notification settings - Fork 741
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
add missing fields to get blob sidecars request #5987
add missing fields to get blob sidecars request #5987
Conversation
…d-missing-fields-get-blob-sidecars
…d-missing-fields-get-blob-sidecars
a0e5092
to
81e6879
Compare
fetch from cache if its a head block
81e6879
to
969e4c7
Compare
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 on the whole, just one small issue that I think we should fix before merging
Tested and can confirm that the fields |
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.
http crate is not my expertise but can't find any issues with the current approach :)
let block = BlockId::blinded_block_by_root(&root, chain)?.ok_or_else(|| { | ||
warp_utils::reject::custom_not_found(format!("beacon block with root {}", root)) | ||
})?; | ||
|
||
// Return the `BlobSidecarList` identified by `self`. | ||
let blob_sidecar_list = chain |
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.
Now that we're loading the block and blobs together, it could be good to check that all the blobs are loaded, and return a 404 in case they aren't
Related to this issue:
I added (and then removed) the backwards-incompat
tag because I thought this change may already accomplish this, but we don't have the check (yet), so it doesn't.
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 0429158 |
* add missing fields to get blob sidecars request * add fork versioned response impl * only compute the block root once * Merge branch 'unstable' of https://github.com/sigp/lighthouse into add-missing-fields-get-blob-sidecars * Merge branch 'unstable' of https://github.com/sigp/lighthouse into add-missing-fields-get-blob-sidecars * fetch root first fetch from cache if its a head block * fmt * always load from db
Issue Addressed
Closes #5977
Proposed Changes
Adds missing fields to get blob sidecars request