Simplify missing cell handling in recovery#629
Conversation
src/common/fr.c
Outdated
| blst_uint64_from_fr(a, p); | ||
| return a[0] == 1 && a[1] == 0 && a[2] == 0 && a[3] == 0; | ||
| bool fr_is_one(const fr_t *fr) { | ||
| return memcmp(fr, &FR_ONE, sizeof(fr_t)) == 0; |
There was a problem hiding this comment.
This switch to memcmp is not truly straightforward because the old approach (using blst_uint64_from_fr()) was doing a modular reduction on the input field element. Whereas now we don't, and this could be a problem if this function ever receives an unreduced field element.
I don't think this is a problem, because we should not be handling unreduced field elements, but it's a new assumption that should be taken into account every time we parse or introduce field elements in any way.
src/common/fr.c
Outdated
| bool fr_is_null(const fr_t *p) { | ||
| return fr_equal(p, &FR_NULL); | ||
| bool fr_is_null(const fr_t *fr) { | ||
| return memcmp(fr, &FR_NULL, sizeof(fr_t)) == 0; |
There was a problem hiding this comment.
Hmm, FR_NULL is only used during recovery and nowhere else. Perhaps we can just kill FR_NULL completely, and initialize recovered_cells_fr with FR_ZERO. After all, the missing cells will be set to zero by multiplication with vanishing_poly_eval. This is what's done in the spec right now (see extended_evaluation_rbo).
There was a problem hiding this comment.
Oh you're right. I like this suggestion. Let me do this.
src/common/fr.h
Outdated
| bool fr_is_one(const fr_t *p); | ||
| bool fr_is_null(const fr_t *p); | ||
| bool fr_is_zero(const fr_t *fr); | ||
| bool fr_is_one(const fr_t *fr); |
There was a problem hiding this comment.
Oops good catch, removed.
asn-d6
left a comment
asn-d6
left a comment
There was a problem hiding this comment.
The recovery part seems good to me. I still don't like fr_equal() using memcmp.
Can't we just keep the recovery improvement?
jtraglia
changed the title
4 participants
Originally, this PR was an optimization in field element comparisons which resulted in a 3% speed-up in
compute_cells_and_kzg_proofsbut the team was concerned with the changes. So we decided to revert the optimization changes and keep the simplification we noticed.