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

Add EIP4844 pylint and Mypy checks #3134

Merged
merged 3 commits into from
Dec 1, 2022
Merged

Add EIP4844 pylint and Mypy checks #3134

merged 3 commits into from
Dec 1, 2022

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Nov 28, 2022

Issue

eip4844 was not enabled in our pylint and Mypy lint command.

Fixes

  • Makefile: Add EIP4844 pylint and Mypy checks
  • specs: fix linter errors
    • Note: updated many int <> BLSFieldElement conversions in this PR. /cc @asn-d6
  • setup.py spec builder:
    1. Polynomial: to make Mypy happy, using class Polynomial(Vector[BLSFieldElement, FIELD_ELEMENTS_PER_BLOB]): pass rather than Polynomial = Vector[BLSFieldElement, FIELD_ELEMENTS_PER_BLOB].
    2. ??? Blob: somehow Mypy throws an error "Invalid base class "ByteVector"" at class Blob(ByteVector[BYTES_PER_FIELD_ELEMENT * FIELD_ELEMENTS_PER_BLOB]): pass.
      • I'm not sure why Vector is acceptable but ByteVector is invalid to Mypy here. They are both dynamic base classes that Mypy might not be able to handle. My guess is that remerkleable might have ignored types in Vector class implementation...? Right now my workaround is manually adding type: ignore in the ByteVector custom type definitions. /cc @protolambda, you may have some insights on it.

@hwwhww hwwhww added scope:CI/tests/pyspec Deneb was called: eip-4844 labels Nov 28, 2022
Comment on lines +179 to +182
# 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +289 to +290
def process_blob_kzg_commitments(state: BeaconState, body: BeaconBlockBody) -> None:
# pylint: disable=unused-argument
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

@kevaundray kevaundray Nov 28, 2022

Choose a reason for hiding this comment

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

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])
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

@kevaundray
Copy link
Contributor

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:

bls_modular_mul
bls_modular_sub
bls_modular_add
bls_modular_div

@hwwhww
Copy link
Contributor Author

hwwhww commented Nov 29, 2022

@kevaundray Thanks for the review!

Yeah, and a helper int_to_field_element(int: x) -> BLSFieldElement may improve readability greatly.

@hwwhww hwwhww requested a review from asn-d6 November 30, 2022 06:00
Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

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.

@hwwhww hwwhww merged commit 602f846 into dev Dec 1, 2022
@hwwhww hwwhww deleted the eip4844-lint branch December 1, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844 scope:CI/tests/pyspec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants