Add EIP4844 pylint and Mypy checks #3134
Conversation
| # For testing, `retrieve_blobs_sidecar` returns "TEST. | ||
| # TODO: Remove it once we have a way to inject `BlobsSidecar` into tests. | ||
| if isinstance(sidecar, str): | ||
| return True |
There was a problem hiding this comment.
If we don't do such a check, Mypy will raise an error at validate_blobs_sidecar(slot, beacon_block_root, blob_kzg_commitments, sidecar):
Argument 4 to "validate_blobs_sidecar" has incompatible type "Union[BlobsSidecar, str]"; expected "BlobsSidecar"
Having isinstance in the spec is indeed unwelcome, but I think it's acceptable if it's labeled as something we will fix eventually. Plus, I noticed that #3125 tempts to remove the whole function.
| def process_blob_kzg_commitments(state: BeaconState, body: BeaconBlockBody) -> None: | ||
| # pylint: disable=unused-argument |
There was a problem hiding this comment.
Keeping state here because I want to re-use the test format. But I'm open to other ideas.
| @@ -144,7 +144,7 @@ def bytes_to_bls_field(b: Bytes32) -> BLSFieldElement: | |||
| """ | |||
| Convert 32-byte value to a BLS field scalar. The output is not uniform over the BLS field. | |||
| """ | |||
| return int.from_bytes(b, ENDIANNESS) % BLS_MODULUS | |||
| return BLSFieldElement(int.from_bytes(b, ENDIANNESS) % BLS_MODULUS) | |||
There was a problem hiding this comment.
Looks good, should return a BLSFieldElement which is an integer < BLS_MODULUS
| @@ -210,7 +210,7 @@ def bls_modular_inverse(x: BLSFieldElement) -> BLSFieldElement: | |||
| Compute the modular inverse of x | |||
| i.e. return y such that x * y % BLS_MODULUS == 1 and return 0 for x == 0 | |||
| """ | |||
| return pow(x, -1, BLS_MODULUS) if x != 0 else 0 | |||
| return BLSFieldElement(pow(x, -1, BLS_MODULUS)) if x != 0 else BLSFieldElement(0) | |||
There was a problem hiding this comment.
Looks good, for reference as a multiline statement this is:
if x == 0:
return BLSFieldElement(0)
return BLSFieldElement(pow(x, -1, BLS_MODULUS))
| @@ -220,7 +220,7 @@ def div(x: BLSFieldElement, y: BLSFieldElement) -> BLSFieldElement: | |||
| """ | |||
| Divide two field elements: ``x`` by `y``. | |||
| """ | |||
| return (int(x) * int(bls_modular_inverse(y))) % BLS_MODULUS | |||
| return BLSFieldElement((int(x) * int(bls_modular_inverse(y))) % BLS_MODULUS) | |||
There was a problem hiding this comment.
Looks good, for readability in a separate PR, I'm thinking in a separate PR we can use intermediate variables to avoid the bracket hell
| @@ -251,7 +251,7 @@ def poly_lincomb(polys: Sequence[Polynomial], | |||
| for v, s in zip(polys, scalars): | |||
| for i, x in enumerate(v): | |||
| result[i] = (result[i] + int(s) * int(x)) % BLS_MODULUS | |||
| return [BLSFieldElement(x) for x in result] | |||
| return Polynomial([BLSFieldElement(x) for x in result]) | |||
| @@ -284,7 +284,7 @@ def evaluate_polynomial_in_evaluation_form(polynomial: Polynomial, | |||
| """ | |||
| width = len(polynomial) | |||
| assert width == FIELD_ELEMENTS_PER_BLOB | |||
| inverse_width = bls_modular_inverse(width) | |||
| inverse_width = bls_modular_inverse(BLSFieldElement(width)) | |||
| result = result * (pow(z, width, BLS_MODULUS) - 1) * inverse_width % BLS_MODULUS | ||
| return result | ||
| a = BLSFieldElement(int(polynomial[i]) * int(roots_of_unity_brp[i]) % BLS_MODULUS) | ||
| b = BLSFieldElement((int(BLS_MODULUS) + int(z) - int(roots_of_unity_brp[i])) % BLS_MODULUS) |
There was a problem hiding this comment.
Looks good though its probably good to note the slight change here.
Before it was : int(z) - int(roots_of_unity_brp[i])
Now its (int(BLS_MODULUS) + int(z) - int(roots_of_unity_brp[i])
When taken % BLS_MODULUS, it is the same result, since `int(BLS_MODULUS) % BLS_MODULUS = 0
The reason to do this is because z-roots_of_unity_brp[i]) can be negative and taking that modulo BLS_MODULUS will probably give a negative result.
| y = evaluate_polynomial_in_evaluation_form(polynomial, z) | ||
| polynomial_shifted = [(p - int(y)) % BLS_MODULUS for p in polynomial] | ||
| polynomial_shifted = [BLSFieldElement((int(p) - int(y)) % BLS_MODULUS) for p in polynomial] |
| @@ -392,7 +390,7 @@ def compute_aggregated_poly_and_commitment( | |||
| r_powers, evaluation_challenge = compute_challenges(polynomials, kzg_commitments) | |||
|
|
|||
| # Create aggregated polynomial in evaluation form | |||
| aggregated_poly = Polynomial(poly_lincomb(polynomials, r_powers)) | |||
|
Crypto changes look good to me. Theres quite a bit of verbosity, where we are doing % BLS_MODULUS in a few places. In a separate pr, I think we can clean this up by introducing a few more helper methods, namely:
|
|
@kevaundray Thanks for the review! Yeah, and a helper |
There was a problem hiding this comment.
LGTM. I reviewed all polynomial-commitments.md changes and they all look sensible.
In general, I feel that all the casting between int and BLSFieldElement adds quite a bit of visual clutter that can be confusing to the reader. Establishing some good wrapper functions (similar to the ones suggested by @kevaundray and @hwwhww) would do good progress in removing all the clutter. This can happen as a separate PR though.
Issue
eip4844was not enabled in our pylint and Mypy lint command.Fixes
Makefile: Add EIP4844 pylint and Mypy checksint<>BLSFieldElementconversions in this PR. /cc @asn-d6setup.pyspec builder:Polynomial: to make Mypy happy, usingclass Polynomial(Vector[BLSFieldElement, FIELD_ELEMENTS_PER_BLOB]): passrather thanPolynomial = Vector[BLSFieldElement, FIELD_ELEMENTS_PER_BLOB].Blob: somehow Mypy throws an error "Invalid base class "ByteVector"" atclass Blob(ByteVector[BYTES_PER_FIELD_ELEMENT * FIELD_ELEMENTS_PER_BLOB]): pass.Vectoris acceptable butByteVectoris invalid to Mypy here. They are both dynamic base classes that Mypy might not be able to handle. My guess is thatremerkleablemight have ignored types inVectorclass implementation...? Right now my workaround is manually addingtype: ignorein theByteVectorcustom type definitions. /cc @protolambda, you may have some insights on it.