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

Integrate new CosetEvals type #3701

Merged
merged 17 commits into from
Apr 22, 2024
Merged

Integrate new CosetEvals type #3701

merged 17 commits into from
Apr 22, 2024

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Apr 18, 2024

This PR renames Cell to CosetEvals and adds a new Cell type which is a flat array of bytes, like Blob. The old Cell type was an abstraction leak (leaking BLS field elements) & the input bytes-representation wasn't a flat array of bytes; it was Vector[Bytes32, FIELD_ELEMENTS_PER_CELL] which is difficult to work with.

@jtraglia jtraglia added the EIP-7594 PeerDAS label Apr 18, 2024
@asn-d6
Copy link
Contributor

asn-d6 commented Apr 19, 2024

Hmm, I'm not a huge fan of CellBytes as a name, especially if it's input/output to public methods. This can be seen in functions like compute_cells_and_proofs() or compute_cells() where they output CellBytes even though the naming implies Cell.

I agree with Justin that ideally we would make Cell a flat array of bytes (like Blob) that is used in public methods, and then figure out how to handle the internal Fr representation that is never exposed to the world (like Polynomial).

With regards to the internal Fr representation, our job here seems harder than the Blob -> Polynomial naming in 4844. I guess an accurate naming could be Cell -> CosetEvals but this might create more confusion than help (altho it might help us create a better narrative around cosets in the codebase).

Another (IMO worse) approach is to just handle the internal Fr representation with an untyped Sequence[BLSFieldElement] which is already what verify_kzg_proof_multi_impl() and compute_kzg_proof_multi_impl() are doing, even though they should be handling Cell.

@jtraglia jtraglia changed the title Add CellBytes type Integrate new CosetEvals type Apr 19, 2024
@kevaundray
Copy link
Contributor

Yeah I think the naming can be improved, approved because we can iterate, and the important thing in this PR is removing internal cryptography types from the API

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

There are still a few issues around the PR. I'm fine with merging it as is, and iterating it on it, if you feel that's the right move.

specs/_features/eip7594/das-core.md Outdated Show resolved Hide resolved
@@ -370,7 +391,7 @@ def coset_for_cell(cell_id: CellID) -> Cell:
roots_of_unity_brp = bit_reversal_permutation(
compute_roots_of_unity(FIELD_ELEMENTS_PER_EXT_BLOB)
)
return Cell(roots_of_unity_brp[FIELD_ELEMENTS_PER_CELL * cell_id:FIELD_ELEMENTS_PER_CELL * (cell_id + 1)])
return CosetEvals(roots_of_unity_brp[FIELD_ELEMENTS_PER_CELL * cell_id:FIELD_ELEMENTS_PER_CELL * (cell_id + 1)])
Copy link
Contributor

@asn-d6 asn-d6 Apr 22, 2024

Choose a reason for hiding this comment

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

Something wrong here with the function wants to return an evaluation domain but we are returning CosetEvals

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed bc607ec in an attempt to clarify things

Copy link
Member Author

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Can't approve my own PR. But this LGTM, thanks for the help guys!

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks all!

@asn-d6 asn-d6 merged commit bcd0a09 into ethereum:dev Apr 22, 2024
28 checks passed
@jtraglia jtraglia deleted the cell-bytes-type branch April 22, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants