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

recover_cells_and_kzg_proofs & matrix refactor #3788

Merged
merged 9 commits into from
Jun 11, 2024

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Jun 4, 2024

This PR does the following:

  • Rename recover_all_cells to recover_cells_and_kzg_proofs.
  • Add proof recovery to the bottom of the function.
  • Replace ExtendedMatrix and DataColumn types with MatrixEntry container.
  • Replace cells_dict with sequence of MatrixEntry values.
  • Update reference tests to work with new changes.

@jtraglia jtraglia requested review from asn-d6 and hwwhww June 4, 2024 23:50
@jtraglia jtraglia added the EIP-7594 PeerDAS label Jun 4, 2024
@jtraglia jtraglia marked this pull request as ready for review June 5, 2024 01:16
@asn-d6
Copy link
Contributor

asn-d6 commented Jun 5, 2024

Code looks good!

BTW, why did you "Replace ExtendedMatrix and DataColumn types with MatrixEntry container."? It seems like this was not necessary. No strong opinion, just wondering. I kinda liked ExtendedMatrix as a type for the full matrix.

@jtraglia
Copy link
Member Author

jtraglia commented Jun 5, 2024

why did you "Replace ExtendedMatrix and DataColumn types with MatrixEntry container."?

Good question. I actually wanted to keep the types, but ran into remerkable issues. I tried doing this:

- List[Cell, MAX_CELLS_IN_EXTENDED_MATRIX]
+ List[Tuple[Cell, KZGProof], MAX_CELLS_IN_EXTENDED_MATRIX]

And it had this error:

telegram-cloud-photo-size-1-5116591456394523592-y

Spent some time trying to fix it but couldn't figure it out.

@asn-d6
Copy link
Contributor

asn-d6 commented Jun 6, 2024

why did you "Replace ExtendedMatrix and DataColumn types with MatrixEntry container."?

Good question. I actually wanted to keep the types, but ran into remerkable issues. I tried doing this:

- List[Cell, MAX_CELLS_IN_EXTENDED_MATRIX]
+ List[Tuple[Cell, KZGProof], MAX_CELLS_IN_EXTENDED_MATRIX]

And it had this error:

telegram-cloud-photo-size-1-5116591456394523592-y

Spent some time trying to fix it but couldn't figure it out.

Hmm, might be worth looking into fixing this.

Right now DataColumn is featured prominently in p2p-interface.md as well. So if we stick with the "untyped" approach of this PR, we should think of how to revise the p2p layer.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

I like this refactor!

  • I don't have a strong feeling about making ColumnIndex an alias of CelIID by inheritance (8c4a42c). Probably not worth introducing a new hardcode in the spec builder?
  • +1 on renaming CellID to CellIndex because they are consecutive numbers. If so, we can leave "ID" for the nonconsecutive identifiers like NodeID.

@jtraglia
Copy link
Member Author

jtraglia commented Jun 7, 2024

  • I don't have a strong feeling about making ColumnIndex an alias of CelIID by inheritance

I don't have a strong opinion on this either. Maybe we should wait a little while and/or do it in a different PR.

  • +1 on renaming CellID to CellIndex

Great, agreed! Unless you want me to do it here, I would like to do this in another PR. It'll be easy, just a lot of changes.

@jtraglia jtraglia changed the title Add proof recovery & matrix clean up recover_cells_and_kzg_proofs & matrix refactor Jun 7, 2024
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!

@asn-d6 asn-d6 merged commit 5ace424 into ethereum:dev Jun 11, 2024
26 checks passed
@jtraglia jtraglia deleted the recover_proofs_too branch June 11, 2024 11:53
b-wagn added a commit to b-wagn/consensus-specs that referenced this pull request Jun 11, 2024
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.

None yet

3 participants