Refactor to prepare for multiproof implementation #40
Conversation
There was a problem hiding this comment.
Nice improvement 👍
I think I've spotted a bug: the bit check no longer covers the entire range of the encoded measurement that needs to be bit-checked. Please double check and let me know if I've missed something.
Stepping back a little, I think it would be really useful to put a diagram some where (in the docucomment for eval()?) of how the encoding works and make sure all of the variables in all of the vunctions are consistent. Something like this (using || to denote concatenation, also correct me if I'm wrong about anything, this isn't totally fresh in my mind):
encoded_measurement =
encoded_gradient || # self.dimension
bit_checked = (
norm_bits || # self.num_bits_for_norm
wr_test_bits || # self.num_bits_for_wr_res * self.NUM_WR_CHECKS
wr_test_results # self.NUM_WR_CHECKS
) ||
wr_dot_prods # self.NUM_WR_CHECKS
)
Also, it would be useful to make sure we use variable names consistenly.
junyechen1996
force-pushed
the
junyec/vdaf-prep
branch
from
f36126b to
b9eddf2
Compare
junyechen1996
force-pushed
the
junyec/multiproof-refactor
branch
from
0f0807e to
730891f
Compare
There was a problem hiding this comment.
I think I've spotted a bug
You are right. It should be fixed now.
Stepping back a little, I think it would be really useful to put a diagram some where
That's a good idea. I put that in eval() function now.
Also, it would be useful to make sure we use variable names consistenly.
Makes sense. I made a pass to call the output of encode(): encoded_gradient (although it's not completely accurate, it also contains the bits of L2-norm check). The output of run_wr_checks are the wr_bits and wr_dot_prods, which are used throughout the validity circuit and VDAF. Note wr_bits contains the wraparound check result bits, and also the success bits.
@cjpatton The only open thread: https://github.com/junyechen1996/draft-chen-cfrg-vdaf-pine/pull/40/files#r1430770039. I'm not sure about which variable you are referring to here...
poc/flp_pine.py
Outdated
| (wr_test_check_res, wr_success_count_check_res) = self.wr_check( | ||
| x, | ||
| bit_checked, | ||
| wr_bits, |
There was a problem hiding this comment.
Suggestion: I think wr_test_bits is as bit more meaningful.
| wr_bits, | |
| wr_test_bits, |
There was a problem hiding this comment.
I'm going to call it wr_check_bits, since we've been using "check" so far. We can change it to "test" if you are more used to that.
Compute dot products before Flp.eval(), so we don't have to persist all wraparound joint randomness in memory, and don't have to repeatedly sample them in each invocation of Flp.eval(), because the dot products don't change within in each proof.
junyechen1996
force-pushed
the
junyec/multiproof-refactor
branch
from
730891f to
67ff536
Compare
@cjpatton Thanks for the feedback! Those are really helpful. |
Compute dot products before Flp.eval(), so we don't have to persist all wraparound joint randomness in memory, and don't have to repeatedly sample them in each invocation of Flp.eval(), because the dot products don't change within in each proof.
Reviewer's note: stacked PR on #38 .