Skip to content
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

Split get_data_column_sidecars parameters #3872

Closed

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Aug 8, 2024

In the data-availability-sampling channel in the Eth R&D server, Potuz brought this up:

I have a little problem with peer DAS that it's not specification related but rather implementation. In many parts of Prysm code we assume that the commitments come in the beacon block. Within ePBS this is not true. Working on top of Electra requires us to change the DA code to take the commitments instead of the blocks on most helpers. Peer DAS is a complication cause clients keep a separate branch for it. So ePBS is explicitly in conflict with peer DAS at the time of merging. Could it be possible to clean the spec to make minimal assumptions that the commitments are in blocks and have the helpers take them directly rather than accessing them from blocks?

@potuz would something like this work for you? Only get_data_column_sidecars needs to change, right?

Thanks for getting back to me Justin, yeah those changes are the kind of things that I'd like to see. I Don't follow the peer das spec to know where else it appears and surely clients will have their own separate signatures on implementations. I would just ask to err on the safe side and take the commitments as you did there (and if it appears somewhere else). This will at least make my life easier when I need to rebase 7732 on top of peer DAS. Otherwise you'll have me pushing to ship both together 😝

I believe get_data_column_sidecars is the only pain-point. This PR splits the signed_block parameter into three: signed_block_header, blob_kzg_commitments , and kzg_commitments_inclusion_proof .

@jtraglia jtraglia added the EIP-7594 PeerDAS label Aug 8, 2024
@hwwhww hwwhww added the general:presentation Presentation (as opposed to content) label Aug 13, 2024
@hwwhww hwwhww mentioned this pull request Aug 20, 2024
3 tasks
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

this feels like something we don't necessarily need to reify in the specs (as clients are free to implement it this way today w/ Deneb, later in Electra, and beyond) but if it is going to substantially streamline development elsewhere I'd accept it

(although im also not sure how much it will streamline if all we have to do is change this one function signature)

@ppopth
Copy link
Member

ppopth commented Oct 1, 2024

Can you point specifically where in the ePBS so that the description will be more complete?

Working on top of Electra requires us to change the DA code to take the commitments instead of the blocks on most helpers

Could you give some examples of such DA code?

@jtraglia
Copy link
Member Author

jtraglia commented Oct 1, 2024

Could you give some examples of such DA code?

cc @potuz

@jtraglia
Copy link
Member Author

This has gone stale. Going to close it. Happy to reopen if there's a need.

@jtraglia jtraglia closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS general:presentation Presentation (as opposed to content)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants