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.
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
DataColumnSidecarsByRoot
req/resp protocol #5196Add
DataColumnSidecarsByRoot
req/resp protocol #5196Changes from 5 commits
5becc19
f14e9de
dd15bb3
c55e959
64061e2
4f3b243
e707536
bb1b416
04c82a3
3a96784
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think match statements are executed in order from left to right. There might be a weird edge case here where both data columns and blobs are avail, but we return 0. If we're trying to support both blobs and data_columns initially, this edge case has the potential to cause issues
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.
Yep, you're right - perhaps we can add the
BlockAndBlobsAndDataColumns
variant to the other arm?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.
So this type is used for RPC responses, so i don't think blobs and data column will coexist - we either request for
block + blobs
orblock + data columns
during lookups and sync, depending on the fork we're at.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.
What happens here if we have both blobs and data_columns available? I guess we'd return a
BlockAndBlobs
variant? Would that imply that if blobs are avail, we'll just skip the data sampling checks?would a
BlockAndBlobsAndDataColumns
variant allow us to support bothblobs
anddata_columns
until were ready to migrate away from blob subnets completely?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.
Yeah, so originally I was thinking to prioritise blobs, but then it's kind of pointless because we aren't executing the data column code paths.
I've been thinking about this and talking to Sean about this as well. A potentially way to introduce this and test it out may be add column subnets and sampling (without making it mandatory for
is_data_avaialble
, perhaps log warnings if it fails), and keep the existing blob subnets and DA filter logic - so perhaps an intermediateBlockAndBlobsAndDataColumns
variant may make sense until we fully switch over to 1D PeerDAS.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.
Can you
so the enum type guarantees that blobs and columns do not exist at the same time
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.
Not sure if I'm understanding correctly but
self
is actually a struct type which contains optional blobs and columns:lighthouse/beacon_node/beacon_chain/src/data_availability_checker.rs
Lines 603 to 608 in e707536
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.
For an
RpcBlock
, we will have eitherblock + blobs
orblock + samples
if we've transitioned to DAS.