EIP7594: Universal Verification in verify_cell_kzg_proof_batch #3812
Conversation
…zg_proof_batch_challenge
There was a problem hiding this comment.
This is a really great start @b-wagn! This spec implementation is quite clear & easy to read. I support adding the universal verification to the spec for the same reasons you listed. While this was only a quick initial review, I only noticed small nits. And thanks so much for cleaning up @kevaundray's trailing whitespace 😄
| The list of all commitments is provided in row_commitments_bytes. | ||
|
|
||
| This function implements the universal verification equation introduced here: | ||
| https://ethresear.ch/t/a-universal-verification-equation-for-data-availability-sampling/13240 |
There was a problem hiding this comment.
Perhaps let's link to the relevant func instead of the blog post here. The main func should link to the blog post.
There was a problem hiding this comment.
What do you mean by relevant func in this context?
Sure, I will add the link to the blog post to verify_cell_kzg_proof_batch.
There was a problem hiding this comment.
I meant to say that we are currently linking to the ethresearch post three times throughout the file. Perhaps we should only link once. Maybe this is the right function doc to do so, and we should remove it from the other function docs. (Sorry for the confusion, I meant to add a comment in a different function doc.)
There was a problem hiding this comment.
Great work here! Thanks a lot!
Left a bunch of comments. Most of them are nitpicks that can be ignored.
I do think we should think a bit whether the concepts of k and k_i in the impl function and their connection to row_indices and column_indices can be clarified further.
| The list of all commitments is provided in row_commitments_bytes. | ||
|
|
||
| This function implements the universal verification equation introduced here: | ||
| https://ethresear.ch/t/a-universal-verification-equation-for-data-availability-sampling/13240 |
There was a problem hiding this comment.
I meant to say that we are currently linking to the ethresearch post three times throughout the file. Perhaps we should only link once. Maybe this is the right function doc to do so, and we should remove it from the other function docs. (Sorry for the confusion, I meant to add a comment in a different function doc.)
Summary
This PR adds an implementation of the universal verification equation for verifying multiple cell KZG proofs.
Before, the spec recommended to use the universal verification equation in a comment, but the code specified verifying proofs individually.
The following has been changed:
verify_cell_kzg_proof_batchverify_cell_kzg_proof_batch_implthat is called byverify_cell_kzg_proof_batchcoset_shift_for_cellto get the shift that specifies a cosetverify_cell_kzg_proof_batch_challengeto get the challenge used for random linear combinationMotivation
Arguably one of the most important if not the most important thing to do right in terms of security for PeerDAS is how to verify cell proofs. To me, it feels dangerous to leave it to the implementors to read the and implement it correctly, given that verification is that important. In other words, I don't believe it is a good idea (especially for verification) that we actively cause a gap between what is specified and what will be implemented.
I would be happy to hear your feedback before investing more time.
Notes
I did not implement the efficient way of computing the combined interpolation polynomial (Appendix of the blogpost) and instead used a fully equivalent naive way of doing this. The rationale behind this is that we do not want to make the spec to complicated. At the same time, the "random linear combination" part is important to have in the spec, as it is no longer fully equivalent to a naive implementation that verifies each proof individually. Concretely, it really changes the success probability of an adversary (by a negligible amount).