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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ install_test:
# Testing against `minimal` config by default
test: pyspec
. venv/bin/activate; cd $(PY_SPEC_DIR); \
python3 -m pytest -n 4 --disable-bls --cov=eth2spec.phase0.minimal --cov=eth2spec.altair.minimal --cov=eth2spec.bellatrix.minimal --cov=eth2spec.capella.minimal --cov-report="html:$(COV_HTML_OUT)" --cov-branch eth2spec
python3 -m pytest -n 4 --disable-bls --cov=eth2spec.phase0.minimal --cov=eth2spec.altair.minimal --cov=eth2spec.bellatrix.minimal --cov=eth2spec.capella.minimal --cov=eth2spec.eip4844.minimal --cov-report="html:$(COV_HTML_OUT)" --cov-branch eth2spec

# Testing against `minimal` config by default
find_test: pyspec
. venv/bin/activate; cd $(PY_SPEC_DIR); \
python3 -m pytest -k=$(K) --disable-bls --cov=eth2spec.phase0.minimal --cov=eth2spec.altair.minimal --cov=eth2spec.bellatrix.minimal --cov=eth2spec.capella.minimal --cov-report="html:$(COV_HTML_OUT)" --cov-branch eth2spec
python3 -m pytest -k=$(K) --disable-bls --cov=eth2spec.phase0.minimal --cov=eth2spec.altair.minimal --cov=eth2spec.bellatrix.minimal --cov=eth2spec.capella.minimal --cov=eth2spec.eip4844.minimal --cov-report="html:$(COV_HTML_OUT)" --cov-branch eth2spec

citest: pyspec
mkdir -p $(TEST_REPORT_DIR);
Expand Down Expand Up @@ -142,8 +142,8 @@ codespell:
lint: pyspec
. venv/bin/activate; cd $(PY_SPEC_DIR); \
flake8 --config $(LINTER_CONFIG_FILE) ./eth2spec \
&& pylint --disable=all --enable unused-argument ./eth2spec/phase0 ./eth2spec/altair ./eth2spec/bellatrix ./eth2spec/capella \
&& mypy --config-file $(LINTER_CONFIG_FILE) -p eth2spec.phase0 -p eth2spec.altair -p eth2spec.bellatrix -p eth2spec.capella
&& pylint --rcfile $(LINTER_CONFIG_FILE) ./eth2spec/phase0 ./eth2spec/altair ./eth2spec/bellatrix ./eth2spec/capella ./eth2spec/eip4844 \
&& mypy --config-file $(LINTER_CONFIG_FILE) -p eth2spec.phase0 -p eth2spec.altair -p eth2spec.bellatrix -p eth2spec.capella -p eth2spec.eip4844

lint_generators: pyspec
. venv/bin/activate; cd $(TEST_GENERATORS_DIR); \
Expand Down
5 changes: 5 additions & 0 deletions linter.ini
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,8 @@ warn_unused_configs = True
warn_redundant_casts = True

ignore_missing_imports = True

# pylint
[MESSAGES CONTROL]
disable = all
enable = unused-argument
21 changes: 8 additions & 13 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ def notify_forkchoice_updated(self: ExecutionEngine,
pass

def get_payload(self: ExecutionEngine, payload_id: PayloadId) -> ExecutionPayload:
# pylint: disable=unused-argument
raise NotImplementedError("no default block production")


Expand Down Expand Up @@ -643,12 +644,14 @@ def sundry_functions(cls) -> str:


def no_op(fn): # type: ignore
# pylint: disable=unused-argument
def wrapper(*args, **kw): # type: ignore
return None
return wrapper


def get_empty_list_result(fn): # type: ignore
# pylint: disable=unused-argument
def wrapper(*args, **kw): # type: ignore
return []
return wrapper
Expand All @@ -663,7 +666,8 @@ def wrapper(*args, **kw): # type: ignore
# End
#

def retrieve_blobs_sidecar(slot: Slot, beacon_block_root: Root) -> Optional[BlobsSidecar]:
def retrieve_blobs_sidecar(slot: Slot, beacon_block_root: Root) -> PyUnion[BlobsSidecar, str]:
# pylint: disable=unused-argument
return "TEST"'''

@classmethod
Expand All @@ -682,8 +686,8 @@ def hardcoded_custom_type_dep_constants(cls, spec_object) -> str:
}


def is_spec_defined_type(value: str) -> bool:
return value.startswith(('ByteList', 'Union', 'Vector', 'List'))
def is_byte_vector(value: str) -> bool:
return value.startswith(('ByteVector'))


def objects_to_spec(preset_name: str,
Expand All @@ -696,17 +700,8 @@ def objects_to_spec(preset_name: str,
new_type_definitions = (
'\n\n'.join(
[
f"class {key}({value}):\n pass\n"
f"class {key}({value}):\n pass\n" if not is_byte_vector(value) else f"class {key}({value}): # type: ignore\n pass\n"
for key, value in spec_object.custom_types.items()
if not is_spec_defined_type(value)
]
)
+ ('\n\n' if len([key for key, value in spec_object.custom_types.items() if is_spec_defined_type(value)]) > 0 else '')
+ '\n\n'.join(
[
f"{key} = {value}\n"
for key, value in spec_object.custom_types.items()
if is_spec_defined_type(value)
]
)
)
Expand Down
14 changes: 9 additions & 5 deletions specs/eip4844/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,13 @@ but MUST NOT be considered valid until a valid `BlobsSidecar` has been downloade
def is_data_available(slot: Slot, beacon_block_root: Root, blob_kzg_commitments: Sequence[KZGCommitment]) -> bool:
# `retrieve_blobs_sidecar` is implementation dependent, raises an exception if not available.
sidecar = retrieve_blobs_sidecar(slot, beacon_block_root)
if sidecar == "TEST":
return True # For testing; remove once we have a way to inject `BlobsSidecar` into tests
validate_blobs_sidecar(slot, beacon_block_root, blob_kzg_commitments, sidecar)

# 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
Comment on lines +179 to +182
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.


validate_blobs_sidecar(slot, beacon_block_root, blob_kzg_commitments, sidecar)
return True
```

Expand Down Expand Up @@ -216,7 +219,7 @@ def tx_peek_blob_versioned_hashes(opaque_tx: Transaction) -> Sequence[VersionedH
```python
def verify_kzg_commitments_against_transactions(transactions: Sequence[Transaction],
kzg_commitments: Sequence[KZGCommitment]) -> bool:
all_versioned_hashes = []
all_versioned_hashes: List[VersionedHash] = []
for tx in transactions:
if tx[0] == BLOB_TX_TYPE:
all_versioned_hashes += tx_peek_blob_versioned_hashes(tx)
Expand Down Expand Up @@ -283,7 +286,8 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe
#### Blob KZG commitments

```python
def process_blob_kzg_commitments(state: BeaconState, body: BeaconBlockBody):
def process_blob_kzg_commitments(state: BeaconState, body: BeaconBlockBody) -> None:
# pylint: disable=unused-argument
Comment on lines +289 to +290
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.

assert verify_kzg_commitments_against_transactions(body.execution_payload.transactions, body.blob_kzg_commitments)
```

Expand Down
30 changes: 14 additions & 16 deletions specs/eip4844/polynomial-commitments.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

```

#### `blob_to_polynomial`
Expand Down Expand Up @@ -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))

```

#### `div`
Expand All @@ -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

```

#### `g1_lincomb`
Expand Down Expand Up @@ -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

```

#### `compute_powers`
Expand Down Expand Up @@ -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


# Make sure we won't divide by zero during division
assert z not in ROOTS_OF_UNITY
Expand All @@ -293,9 +293,11 @@ def evaluate_polynomial_in_evaluation_form(polynomial: Polynomial,

result = 0
for i in range(width):
result += div(int(polynomial[i]) * int(roots_of_unity_brp[i]), (int(z) - int(roots_of_unity_brp[i])))
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.

result += int(div(a, b) % BLS_MODULUS)
result = result * int(pow(z, width, BLS_MODULUS) - 1) * int(inverse_width)
return BLSFieldElement(result % BLS_MODULUS)
```

### KZG
Expand Down Expand Up @@ -355,17 +357,13 @@ def compute_kzg_proof(polynomial: Polynomial, z: BLSFieldElement) -> KZGProof:
Compute KZG proof at point `z` with `polynomial` being in evaluation form
Do this by computing the quotient polynomial in evaluation form: q(x) = (p(x) - p(z)) / (x - z)
"""

# To avoid SSZ overflow/underflow, convert element into int
polynomial = [int(i) for i in polynomial]
z = int(z)

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


# Make sure we won't divide by zero during division
assert z not in ROOTS_OF_UNITY
denominator_poly = [(int(x) - z) % BLS_MODULUS for x in bit_reversal_permutation(ROOTS_OF_UNITY)]
denominator_poly = [BLSFieldElement((int(x) - int(z)) % BLS_MODULUS)
for x in bit_reversal_permutation(ROOTS_OF_UNITY)]

# Calculate quotient polynomial by doing point-by-point division
quotient_polynomial = [div(a, b) for a, b in zip(polynomial_shifted, denominator_poly)]
Expand All @@ -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

aggregated_poly = poly_lincomb(polynomials, r_powers)

# Compute commitment to aggregated polynomial
aggregated_poly_commitment = KZGCommitment(g1_lincomb(kzg_commitments, r_powers))
Expand Down
1 change: 1 addition & 0 deletions specs/eip4844/validator.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Implementers may also retrieve blobs individually per transaction.

```python
def get_blobs_and_kzg_commitments(payload_id: PayloadId) -> Tuple[Sequence[BLSFieldElement], Sequence[KZGCommitment]]:
# pylint: disable=unused-argument
...
```

Expand Down