search

sigp / lighthouse

Ethereum consensus client in Rust
https://lighthouse.sigmaprime.io/
Apache License 2.0
2.81k stars 696 forks source link

chore: add `proof` parameter to `recover_cells_and_kzg_proofs` #5998

Closed kevaundray closed 4 days ago

kevaundray commented 5 days ago

Issue Addressed

To recover the cells and proofs, we do not need the proofs that correspond to the cells we already have. As of now the DAS branch recovers_cells_and_proofs using three methods recover_cells, cells_to_blob, compute_cells_and_proofs.

The consensus-specs has introduced a new method which encapsulates all three of these methods, called recover_cells_and_proofs. This method takes in a proof parameter and there are additional checks that libraries that implement that function will perform. This means that the proofs parameter cannot be missing or empty. As an example, libraries will check that the number of proofs is equal to the number of cells.

We will eventually remove the proof parameter here: https://github.com/ethereum/consensus-specs/pull/3819 but until then it is needed.

Proposed Changes

In reconstruct, we also fetch the proof for the given cell.

The specs do it by forming an ExtendedMatrix, however most codebases seem to just have PartialDataColumnSideCars instead, so was not able to corroborate with the specs

Additional Info

Please provide any additional information. For example, future considerations or information useful for reviewers.

kevaundray commented 5 days ago

Updating c-kzg should also surface the errors in the ef-tests and on devnet. In particular, it should refuse to make proofs and unconditionally return an error.

kevaundray commented 5 days ago

Check here for the recover_cells_and_kzg_proofs method in consensus-specs to corroborate that it now takes a proof parameter: https://github.com/ethereum/consensus-specs/blob/dev/specs/_features/eip7594/polynomial-commitments-sampling.md#recover_cells_and_kzg_proofs

kevaundray commented 5 days ago

Note: I have not updated c-kzg in this PR because there are other breaking changes that were added that we would need to modify the codebase for, which would be out of scope, this is just to align the API with the consensus-spec-tests while ensuring nothing breaks.

kevaundray commented 4 days ago

Closing and making the cryptography library ignore the test vectors involving proofs instead