Skip to content

Fix for #7212 Call engine_getBlobs as soon as receiving a valid data_column_sidecar from gossip #7329

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

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

SunnysidedJ
Copy link

@SunnysidedJ SunnysidedJ commented Apr 17, 2025

Issue Addressed

#7212 Call engine_getBlobs as soon as receiving a valid data_column_sidecar from gossip

Proposed Changes

Copied how blobs are fetched when a block is gossiped. Test in progress, and code will be refactored to merge common code between fetching logics from block and column gossips

Additional Info

Please find the comments below

@SunnysidedJ SunnysidedJ requested a review from jxs as a code owner April 17, 2025 01:33
@SunnysidedJ
Copy link
Author

Lock while fetching from EL yet enabled

@SunnysidedJ SunnysidedJ changed the title Fix for #7228 Adding store tests for data column pruning Fix for #7212 Call engine_getBlobs as soon as receiving a valid data_column_sidecar from gossip Apr 17, 2025
@eserilev eserilev added das Data Availability Sampling fulu Required for the upcoming Fulu hard fork labels Apr 17, 2025
@pawanjay176 pawanjay176 self-requested a review April 21, 2025 22:19
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

This looks good functionally.
However, I think there is scope to reduce duplication between the fetch_blobs_from_block v/s fetch_blobs_from_columns functions by simply changing the arguments to the block version and reusing them in the data_column functions.

Repeating the logic opens up to more bugs during further refactors

@pawanjay176
Copy link
Member

pawanjay176 commented Apr 21, 2025

@SunnysidedJ I added a commit based on your branch to illustrate my suggestion 6fe3c5b

Let me know if you disagree with the approach. Feel free to cherry-pick the commit to this PR

@jimmygchen jimmygchen added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Apr 30, 2025
Copy link

mergify bot commented May 19, 2025

Some required checks have failed. Could you please take a look @SunnysidedJ? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling fulu Required for the upcoming Fulu hard fork waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants