Update recovery code to match specs #459
Conversation
There was a problem hiding this comment.
Thanks for the PR, this is great!
Unifying the code with the spec is always good.
In this particular case, it also makes it much more readable and removes more complex functions.
I left a few comments, mostly about how to make it even more readable, how to deal with edge cases, and possibly one improvement.
Let me know what you think :)
| * are missing indices, using a combination of direct multiplication | ||
| * (#do_zero_poly_mul_partial) and iterated multiplication via convolution | ||
| * (#reduce_partials). | ||
| * The roots of unity are chosen based on the missing cell indices. If the i'th |
There was a problem hiding this comment.
I think the doc comment (and comments in the function itself) here is already pretty good.
One thing I would add is where to get the i'th root of unity from, as this heavily depends on the order of the roots.
If I understand correctly, it would be s->expanded_roots_of_unity[i]?
There was a problem hiding this comment.
Yep I can modify that to be more explicit, and yep it is expanded_roots_of_unity (non-bit-reversed) -- note that the code has been modularized so that all of the vanishing polynomial calculations/functions do not depend on any bit reversal permutation
There was a problem hiding this comment.
I've modified it to include expanded_roots_of_unity, though feel free to push a commit to modify it further
Co-authored-by: Benedikt Wagner <[email protected]>
…is called with no cells or all of the cells
…this method general, but not too much benefit for the complexity in c)
…oreso now that `vanishing_poly_len` is gone
Co-authored-by: Justin Traglia <[email protected]>
| * is trivial. We expect the caller to handle this case, | ||
| * and return C_KZG_BADARGS if not. | ||
| * @remark `missing_cell_indices` are assumed to be less than | ||
| * `CELLS_PER_EXT_BLOB`. |
There was a problem hiding this comment.
| * `CELLS_PER_EXT_BLOB`. | |
| * `CELLS_PER_EXT_BLOB` and at least one. |
There was a problem hiding this comment.
The previous remark should be good enough.
This updates the computation of the zero polynomial to match the specs.