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

Add KZG multi verify function #3236

Merged
merged 22 commits into from
Feb 15, 2023
Merged

Add KZG multi verify function #3236

merged 22 commits into from
Feb 15, 2023

Conversation

dankrad
Copy link
Contributor

@dankrad dankrad commented Jan 27, 2023

This is an addition to the cryptography API to allow for efficient verification in the case where every blob comes with its own commitment proof.

The new function verify_blob_kzg_proof_batch can be called for any number of blobs and verifies their commitments using the individual blobs, commitments, and proofs. It is marginally less efficient than creating an aggregate proof for all blobs together, but not by much (ca. 300 us per blob).

Update: Now changed so that the blob verification protocol only takes single blobs and commitments in order to simplify the protocol. This makes sense because the only application of aggregate verification would now have been transactions, and it makes sense to just change them to one proof per blob since then the proofs can just be copied by block producers (makes local block production cheaper).

@sauliusgrigaitis
Copy link
Contributor

The max blobs per block is 4. This is equal to or less than the number of cores of most of the target CPUs. I would suggest to start adding a comparison with simple parallel alternatives (in this particular case it would do a simple single blob verification per thread) together with such more sophisticated proposals.

In many cases, simple parallel algos are simpler (likely more secure) and faster. Like blob proof generation here.

@hwwhww hwwhww added the Deneb was called: eip-4844 label Feb 9, 2023
@dankrad
Copy link
Contributor Author

dankrad commented Feb 13, 2023

The max blobs per block is 4. This is equal to or less than the number of cores of most of the target CPUs. I would suggest to start adding a comparison with simple parallel alternatives (in this particular case it would do a simple single blob verification per thread) together with such more sophisticated proposals.

In many cases, simple parallel algos are simpler (likely more secure) and faster. Like blob proof generation here.

The max number of blobs per block is not 4. This was a temporary conservative restriction when (a) we were trying to shoot for Shanghai and (b) we did not have any networking tests. This is still likely to be increased.

The spec should not try to specify any parallel algorithms. It just needs to specify what's correct and not provide optimizations.

@dankrad dankrad marked this pull request as ready for review February 13, 2023 14:37
@dankrad
Copy link
Contributor Author

dankrad commented Feb 13, 2023

Marked as ready for review because I think the consensus is to (a) free the blobs and (b) make this change to the cryptography.

specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
@asn-d6 asn-d6 mentioned this pull request Feb 14, 2023
3 tasks
Copy link
Member

@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.

A few things I noticed...

specs/deneb/polynomial-commitments.md Outdated Show resolved Hide resolved
specs/deneb/polynomial-commitments.md Show resolved Hide resolved
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! 🚀

Copy link
Member

@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.

LGTM 👍

@dankrad dankrad merged commit 59129e4 into dev Feb 15, 2023
@hwwhww hwwhww deleted the kzg_multi_verify branch February 16, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants