-
Notifications
You must be signed in to change notification settings - Fork 998
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
Conversation
4ef23fa
to
a4fdff3
Compare
There was a problem hiding this 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.
Points to discuss tomorrow:
|
validate_kzg_g1(commitment) | ||
validate_kzg_g1(proof) | ||
|
||
return verify_kzg_proof_impl(commitment, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
3db5654
to
03ff210
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 forKZGCommitment
andKZGProof
. We now do the validation in an explicit manner.After this patch,
KZGCommitment
andKZGProof
now represent G1 byte arrays that have been validated. They should only be used by internal functions.