junyechen1996 / draft-chen-cfrg-vdaf-pine

VDAF to support aggregating real number vectors with L2-norm bound
Other
5 stars 0 forks source link

poc: wraparound check #28

Closed junyechen1996 closed 1 year ago

junyechen1996 commented 1 year ago
junyechen1996 commented 1 year ago

@cjpatton Thanks for the review. I responded to all your comments (hopefully), let me know if I miss any. Things that I would like you to pay particular attention to:

Responding to the high-level comments:

Please remove every variable from PineValid that is not used by any of the methods.

Done. I will add them back once I implement L2-norm check.

Suggestion: I prefer the name "wraparound test" instead of "repetition". "Repetition" begs the question "what is being repeated"? the only answer to which is "the wraparound test".

I start calling all of them "checks" now, and removed repetitions. I'm not sure if it's worth introducing another term "test", but I'd welcome your feedback.

The wraparound subcircuit should include more comments that help connect the reader to the properties we intend to enforce. (See inline comments.)

I addressed the inline comments and added more code comments. Please take a look again.

junyechen1996 commented 1 year ago

Looking really good! I know it looks like a lot of comments, but they're mostly editorial. However I spotted one potential bug I wanted you to look at, in the wraparound rand derivation.

@cjpatton Thanks for the thoughtful reviews! That is a bug, I've fixed it now. Here are the threads that I'd like you to take a look again. As always, feel free to look over the revised PR again.

junyechen1996 commented 1 year ago

@cjpatton Great suggestion about testing bit chunking. Please take a look at the updated code, which contains some of your code, but with some simplification.

The remaining thread is the one on whether l2_norm_bound should be stored: https://github.com/junyechen1996/draft-chen-cfrg-vdaf-pine/pull/28#discussion_r1326519399