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

EIP4844: All public methods take bytes as input (also added explicit G1 validation) #3224

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

asn-d6
Copy link
Contributor

@asn-d6 asn-d6 commented Jan 24, 2023

This is the next step after #3219 . With this patch all Public methods take bytes as input and do explicit validation (that is, the G1 group validation specified by #3223).

We introduce a new validation function called validate_kzg_g1() which implements the validation rules for KZGCommitment and KZGProof. We now do the validation in an explicit manner.

After this patch, KZGCommitment and KZGProof now represent G1 byte arrays that have been validated. They should only be used by internal functions.

@asn-d6 asn-d6 added the Deneb was called: eip-4844 label Jan 24, 2023
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.

Looking good, I have some minor nits.

specs/eip4844/polynomial-commitments.md Outdated Show resolved Hide resolved
specs/eip4844/polynomial-commitments.md Show resolved Hide resolved
specs/eip4844/polynomial-commitments.md Show resolved Hide resolved
@dankrad
Copy link
Contributor

dankrad commented Jan 24, 2023

Points to discuss tomorrow:

  • In the similar case of handling BLS signatures, we just set BLSSignature = Bytes96. Should we not take the same route here to make types more clear?
  • General discussion about crypto -- should type checks be explicit or implicit (my tendency is to say that explicit checks are best)

validate_kzg_g1(commitment)
validate_kzg_g1(proof)

return verify_kzg_proof_impl(commitment,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be cast explicitly to match the expected types of _impl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed 4204f4c which introduces explicit type conversion between "untrusted" bytes and "trusted" BLS types.

expected_kzg_commitments: Sequence[KZGCommitment],
kzg_aggregated_proof: KZGProof) -> bool:
expected_kzg_commitments_bytes: Sequence[Bytes48],
kzg_aggregated_proof_bytes: Bytes48) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, could we rename these to expected_commitments_bytes & aggregated_proof_bytes, respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d6bb1b9

```


#### `verify_kzg_proof_impl`

```python
def verify_kzg_proof_impl(polynomial_kzg: KZGCommitment,
def verify_kzg_proof_impl(commitment: KZGCommitment,
Copy link
Member

Choose a reason for hiding this comment

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

Also, maybe rename this to expected_commitment for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the opposite -- as discussed -- in 3db5654

@@ -336,35 +379,38 @@ def blob_to_kzg_commitment(blob: Blob) -> KZGCommitment:
#### `verify_kzg_proof`

```python
def verify_kzg_proof(polynomial_kzg: KZGCommitment,
def verify_kzg_proof(commitment_bytes: Bytes48,
z: Bytes32,
y: Bytes32,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't z and y be z_bytes and y_bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to do this rename in the future.

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

LGTM

@asn-d6 asn-d6 merged commit 6e397b1 into ethereum:dev Jan 25, 2023
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.

None yet

6 participants