Skip to content

Conversation

@kevaundray
Copy link
Contributor

Empty blobs and commitments currently cause a panic.

This PR aims to handle this particular edge case.

- Put the code related to computing challenges in another method

- remove panic on poly_lincomb() when `polys` is empty
@kevaundray
Copy link
Contributor Author

Latest commit refers to the review left here by @asn-d6

@asn-d6
Copy link
Contributor

asn-d6 commented Nov 11, 2022

Pushed two commits that resolve the linter error and add an assert here: https://github.com/asn-d6/consensus-specs/tree/pr_3093

All in all, I think the changes are sensible.

Given the details of this special case, I'd like to do another review pass (probably early next week). Also, it would be great if @dankrad could take a look.

@kevaundray kevaundray marked this pull request as ready for review November 11, 2022 16:53
@kevaundray
Copy link
Contributor Author

Pushed two commits that resolve the linter error and add an assert here: https://github.com/asn-d6/consensus-specs/tree/pr_3093

All in all, I think the changes are sensible.

Given the details of this special case, I'd like to do another review pass (probably early next week). Also, it would be great if @dankrad could take a look.

Merged changes into the branch to update ci

@hwwhww hwwhww requested a review from dankrad November 11, 2022 20:25
@hwwhww hwwhww added the deneb label Nov 11, 2022
@hwwhww
Copy link
Contributor

hwwhww commented Nov 16, 2022

@kevaundray the CI errors happend at:

  1. In evaluate_polynomial_in_evaluation_form:
    • Need to cast int(z) in result = result * (pow(z, width, BLS_MODULUS) - 1) * inverse_width % BLS_MODULUS
      -> result = result * (pow(int(z), width, BLS_MODULUS) - 1) * inverse_width % BLS_MODULUS.
  2. In the empty blobs and case, evaluate_polynomial_in_evaluation_form failed when checking assert z not in ROOTS_OF_UNITY

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

This makes sense to me. I think it's much more natural than to have network messages with the absence of a SideCar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants