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

EIP-7594: Specify inclusion proof function to run. #3963

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

nalepae
Copy link
Contributor

@nalepae nalepae commented Oct 7, 2024

For by root and by range requests checks, the exact function to run is specified for KZG commitments, but not for the inclusion proof.

This PR aligns both checks.

For by root and by range requests checks, the exact function to run is specified for KZG commitments, but not for the inclusion proof.

This PR aligns both checks.
@nalepae
Copy link
Contributor Author

nalepae commented Oct 7, 2024

Additional question: Should the SHOULD be replaced by a MUST?

Consequences could be quite dramatic if a client actually considers a block with data column sidecars with invalid inclusion proof and/or invalid KZG proofs as available.

@jtraglia
Copy link
Member

jtraglia commented Oct 8, 2024

Additional question: Should the SHOULD be replaced by a MUST?

Precedent says no. For example, the equivalent check for BlobSidecarsByRoot:

Before consuming the next response chunk, the response reader SHOULD verify the blob sidecar is well-formatted, has valid inclusion proof, and is correct w.r.t. the expected KZG commitments through `verify_blob_kzg_proof`.

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.

Thanks. I like explicitly calling out those functions.

Could we mention verify_data_column_sidecar too?

@@ -223,7 +223,7 @@ Requests sidecars by block root and index.
The response is a list of `DataColumnIdentifier` whose length is less than or equal to the number of requests.
It may be less in the case that the responding peer is missing blocks or sidecars.

Before consuming the next response chunk, the response reader SHOULD verify the data column sidecar is well-formatted, has valid inclusion proof, and is correct w.r.t. the expected KZG commitments through `verify_data_column_sidecar_kzg_proofs`.
Before consuming the next response chunk, the response reader SHOULD verify the data column sidecar is well-formatted, has valid inclusion proof through `verify_data_column_sidecar_inclusion_proof`, and is correct w.r.t. the expected KZG commitments through `verify_data_column_sidecar_kzg_proofs`.
Copy link
Member

@jtraglia jtraglia Oct 8, 2024

Choose a reason for hiding this comment

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

This applies to both lines.

Suggested change
Before consuming the next response chunk, the response reader SHOULD verify the data column sidecar is well-formatted, has valid inclusion proof through `verify_data_column_sidecar_inclusion_proof`, and is correct w.r.t. the expected KZG commitments through `verify_data_column_sidecar_kzg_proofs`.
Before consuming the next response chunk, the response reader SHOULD verify the data column sidecar is well-formatted through `verify_data_column_sidecar`, has valid inclusion proof through `verify_data_column_sidecar_inclusion_proof`, and is correct w.r.t. the expected KZG commitments through `verify_data_column_sidecar_kzg_proofs`.

@jtraglia jtraglia added the EIP-7594 PeerDAS label Oct 10, 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.

2 participants