Skip to content

Commit

Permalink
allow empty blobs:
Browse files Browse the repository at this point in the history
- Put the code related to computing challenges in another method

- remove panic on poly_lincomb() when `polys` is empty
  • Loading branch information
kevaundray committed Nov 11, 2022
1 parent d4a9b97 commit 4a4be2e
Showing 1 changed file with 21 additions and 3 deletions.
24 changes: 21 additions & 3 deletions specs/eip4844/polynomial-commitments.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
- [`g1_lincomb`](#g1_lincomb)
- [`poly_lincomb`](#poly_lincomb)
- [`compute_powers`](#compute_powers)
- [`compute_challenges`](#compute_challenges)
- [Polynomials](#polynomials)
- [`evaluate_polynomial_in_evaluation_form`](#evaluate_polynomial_in_evaluation_form)
- [KZG](#kzg)
Expand Down Expand Up @@ -231,7 +232,7 @@ def poly_lincomb(polys: Sequence[Polynomial],
Given a list of ``polynomials``, interpret it as a 2D matrix and compute the linear combination
of each column with `scalars`: return the resulting polynomials.
"""
result = [0] * len(polys[0])
result = [0] * FIELD_ELEMENTS_PER_BLOB
for v, s in zip(polys, scalars):
for i, x in enumerate(v):
result[i] = (result[i] + int(s) * int(x)) % BLS_MODULUS
Expand All @@ -252,6 +253,24 @@ def compute_powers(x: BLSFieldElement, n: uint64) -> Sequence[BLSFieldElement]:
current_power = current_power * int(x) % BLS_MODULUS
return powers
```
#### `compute_challenges`

```python
def compute_challenges(x: BLSFieldElement, n: uint64) -> Tuple[Sequence[BLSFieldElement], BLSFieldElement]:
"""
Return the random linear combination challenges and the evaluation challenge

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Nov 11, 2022

Might be a good idea to also document what x and n are in this context.

This comment has been minimized.

Copy link
@kevaundray

kevaundray Nov 11, 2022

Author Owner

Yep makes sense

"""
powers = compute_powers(x, n)
evaluation_challenge = 0

# When n == 0, this means that the blobs are empty
# in that case, we define the evaluation challenge to be 0
if len(powers) != 0:

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Nov 11, 2022

Maybe this could be if len(n) != 0:. I think it's the same behavior and closer to the comment. Also no need to understand the internals of compute_powers()

This comment has been minimized.

Copy link
@kevaundray

kevaundray Nov 11, 2022

Author Owner

Yep agreed, this is closer to the comment

This comment has been minimized.

Copy link
@kevaundray

kevaundray Nov 11, 2022

Author Owner

Changed it to n != 0, since n is an integer

This comment has been minimized.

Copy link
@dankrad

dankrad Nov 11, 2022

There is no reason for this weird special casing?

Just set powers = computer_powers(x, n+1) and return powers[:-1], powers[-1]

Don't even really see why it needs it's own function...

evaluation_challenge = int(r_powers[-1]) * r % BLS_MODULUS

return powers, evaluation_challenge

```

### Polynomials

Expand Down Expand Up @@ -351,8 +370,7 @@ def compute_aggregated_poly_and_commitment(

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Nov 11, 2022

Can you please also document this edge-case behavior somewhere? "This function should be able to work with blobs == [] and kzg_commitments == []`"? Maybe on this function? Or somewhere else?

This comment has been minimized.

Copy link
@kevaundray

kevaundray Nov 11, 2022

Author Owner

I've put it on the compute_aggregated_poly_and_commitment let me know if that works

This comment has been minimized.

Copy link
@dankrad

dankrad Nov 11, 2022

I mean this should really be obvious, but ok.

# Generate random linear combination challenges
r = hash_to_bls_field(polynomials, kzg_commitments)
r_powers = compute_powers(r, len(kzg_commitments))
evaluation_challenge = int(r_powers[-1]) * r % BLS_MODULUS
r_powers, evaluation_challenge = compute_challenges(r, len(kzg_commitments))

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Nov 11, 2022

Any chance you could write a unittest for this edge-case to make sure that all the surrounding functions also work as intended?

https://github.com/ethereum/consensus-specs/blob/dev/tests/core/pyspec/eth2spec/test/eip4844/unittests/validator/test_validator.py is a good place for it.

You can run then run the test with: make lint && python -m pytest tests/core/pyspec/eth2spec/test/eip4844/unittests/validator/test_validator.py

This comment has been minimized.

Copy link
@kevaundray

kevaundray Nov 11, 2022

Author Owner

Will just adding a blob_count=0 test here be okay here?

# Create aggregated polynomial in evaluation form
aggregated_poly = Polynomial(poly_lincomb(polynomials, r_powers))
Expand Down

0 comments on commit 4a4be2e

Please sign in to comment.