Skip to content

Return error if getBlobs not supported #6911

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 5, 2025
Merged

Return error if getBlobs not supported #6911

merged 1 commit into from
Feb 5, 2025

Conversation

pawanjay176
Copy link
Member

Issue Addressed

N/A

Proposed Changes

Previously, we were returning an empty vec of Nones if get_blobs was not supported in the EL. This results in confusing logging where we try to process the empty list of blobs and log a bunch of Unexpected errors. See

Feb 03 17:32:12.383 DEBG Fetching blobs from the EL, num_expected_blobs: 6, block_root: 0x7326fe2dc1cb9036c9de7a07a662c86a339085597849016eadf061b70b7815ba, service: fetch_engine_blobs, service: beacon, module: beac
on_chain::fetch_blobs:84
Feb 03 17:32:12.384 DEBG Processing engine blobs, num_fetched_blobs: 0, block_root: 0x7326fe2dc1cb9036c9de7a07a662c86a339085597849016eadf061b70b7815ba, service: fetch_engine_blobs, service: beacon, module: beacon_c
hain::fetch_blobs:197
Feb 03 17:32:12.384 ERRO Error fetching or processing blobs from EL, block_root: 0x7326fe2dc1cb9036c9de7a07a662c86a339085597849016eadf061b70b7815ba, error: BlobProcessingError(AvailabilityCheck(Unexpected)), module
: network::network_beacon_processor:1011

The error we should be getting is that getBlobs is not supported, this PR adds a new error variant and returns that.

@pawanjay176 pawanjay176 added the ready-for-review The code is ready for review label Feb 4, 2025
jimmygchen
jimmygchen approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Nice, LGTM! Thanks

@jimmygchen jimmygchen added das Data Availability Sampling ready-for-merge This PR is ready to merge. labels Feb 5, 2025
mergify bot added a commit that referenced this pull request Feb 5, 2025
@mergify
Merge of #6911
8cfbe49
@mergify mergify bot mentioned this pull request Feb 5, 2025
Closed
6 tasks
@mergify mergify bot merged commit 7bfdb33 into sigp:unstable Feb 5, 2025
31 checks passed
@michaelsproul michaelsproul mentioned this pull request Feb 7, 2025
Merged
mergify bot pushed a commit that referenced this pull request Feb 7, 2025
@michaelsproul
Fix fetch blobs in all-null case (#6940)
cb117f8 
Fix another issue with fetch-blobs, similar to:

- #6911


  Check if the list of blobs returned is all `None`, and if so, do not proceed any further.

This prevents an ugly error like:

> Feb 03 17:32:12.384 ERRO Error fetching or processing blobs from EL, block_root: 0x7326fe2dc1cb9036c9de7a07a662c86a339085597849016eadf061b70b7815ba, error: BlobProcessingError(AvailabilityCheck(Unexpected)), module
: network::network_beacon_processor:1011
@santi1234567 santi1234567 mentioned this pull request Apr 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

Assignees
No one assigned
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge. ready-for-review The code is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pawanjay176 @jimmygchen