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

eip7594: Remove proof parameter from recover_cells_and_kzg_proofs #3819

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

kevaundray
Copy link
Contributor

This removes the proof parameter from recover_cells_and_kzg_proofs since computing individual proofs is only favourable when there are one to five cells missing. Since most clients will start recovery when half of the cells are missing, this functionality would not be useful in practice for our usecases.

We could leave it there, ignore it and have implementations decide, but note that we do need to validate the proofs are valid according to the current API, so its not free.

This means we are essentially validating the proofs twice, since a user should only pass in proofs + cells that they have passed to verify_cell_kzg_proof beforehand. It is not a security concern if these proofs are not validated since the receiver will validate them, but given that fk20 takes about 200ms to compute all of the proofs on a single thread, I don't think it makes sense to try and optimize for the seemingly rare case of one to five missing cells/proofs

@kevaundray
Copy link
Contributor Author

One possible blocker for this PR, is the time it takes to now compute all of the proofs. Before it was only computing the missing proofs.

This is a possible blocker because it increases the CI time.

cc @hwwhww

@kevaundray kevaundray changed the title eip7594: Remove proof parameter from recover_cells_and_kzg_proofs eip7594: Remove proof parameter from recover_cells_and_kzg_proofs Jun 26, 2024
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 👍 I agree with this change. This also removes individual proof computation in recovery. In the real world, I believe it's unlikely individual proof computation will be necessary since nodes will begin recovery as soon as they have 50% of cells. And the optimized FK20 implementations (which compute all of the proofs at once) are so fast, it would only make sense to recover individual proofs if 3 or fewer cells/proofs are missing, which is unlikely.

Also, this highlighted an issue with one of the reference tests. It wasn't updated to include the proof param before, so it was throwing an exception for the wrong reason. Not a big deal, but I'm glad that's okay now.

image

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!

@jtraglia jtraglia added the EIP-7594 PeerDAS label Jun 26, 2024
@jtraglia jtraglia merged commit a3a6c91 into ethereum:dev Jun 27, 2024
26 checks passed
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