Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make FLP Eval Return Vec<F> rather than a single F #1132

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Nov 7, 2024

This makes Type::valid() return Vec<F>, and updates the Type::query() to use query randomness to compress valid() output back to a single group element. This is one of the items in #1122.

I've also taken the liberty of updating the JOINT_RAND_LEN and EVAL_OUTPUT_LEN values to reflect those in draft 12, since draft 10 had EVAL_OUTPUT_LEN all equal to 1, which makes it impossible to test whether things are correct. I kinda took a half-measure here: I updated the Sum type to output bits many elements, but didn't fully do what draft 12 does, and implement the new range check part. I figure we can do this later.

Finally, I simplified Average to use Sum, since they're doing almost entirely the same thing.

This does not pass the known-answer tests. I think we need to regenerate those JSON files. How do I do that?

@rozbb rozbb requested a review from a team as a code owner November 7, 2024 04:10
@divergentdave
Copy link
Contributor

The JSON files are taken from the spec's proof of concept implementation. However, the state of the codebase currently doesn't match up with any draft (we'd still need to change the VERSION constant, make XOF application context changes, etc.). We can just #[ignore] the tests against test vectors from the time being, with a message pointing to #1122.

(P.S. the PR description should say we compress the output with query randomness, not joint randomness)

Copy link
Contributor

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

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

Looking good, just one suggestion

src/flp.rs Outdated
fn query_rand_len(&self) -> usize;
/// circuit, plus the number of elements output by the validity circuit (if >1).
fn query_rand_len(&self) -> usize {
let mut n = self.gadget().len();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since gadget() may do multiple allocations, plus other initialization, and we're a bit liberal about calling query_rand_len(), I think we may want to remove this provided method implementation, and instead just update the constants returned by the existing query_rand_len() implementations.

@divergentdave
Copy link
Contributor

The failing tests are happening because two SZK tests hardcoded that the Sum circuit should use joint randomness, but it no longer does. The following patch should fix them:

diff --git a/src/flp/szk.rs b/src/flp/szk.rs
index ef50420..e25598d 100644
--- a/src/flp/szk.rs
+++ b/src/flp/szk.rs
@@ -904,7 +904,7 @@ mod tests {
         let szk_typ = Szk::new_turboshake128(sum, algorithm_id);
         let prove_rand_seed = Seed::<16>::generate().unwrap();
         let helper_seed = Seed::<16>::generate().unwrap();
-        let leader_seed_opt = Some(Seed::<16>::generate().unwrap());
+        let leader_seed_opt = None;
         let helper_input_share = random_vector(szk_typ.typ.input_len()).unwrap();
         let mut leader_input_share = encoded_measurement.clone().to_owned();
         for (x, y) in leader_input_share.iter_mut().zip(&helper_input_share) {
@@ -944,7 +944,7 @@ mod tests {
         let szk_typ = Szk::new_turboshake128(sum, algorithm_id);
         let prove_rand_seed = Seed::<16>::generate().unwrap();
         let helper_seed = Seed::<16>::generate().unwrap();
-        let leader_seed_opt = Some(Seed::<16>::generate().unwrap());
+        let leader_seed_opt = None;
         let helper_input_share = random_vector(szk_typ.typ.input_len()).unwrap();
         let mut leader_input_share = encoded_measurement.clone().to_owned();
         for (x, y) in leader_input_share.iter_mut().zip(&helper_input_share) {

@rozbb
Copy link
Contributor Author

rozbb commented Nov 11, 2024

Applied your patch. Re calling self.gadget(), I'm a little hesitant to copy the query_rand_len logic everywhere because it's both complicated and redundant. I tried another way, which is to define Type::num_gadgets and state that it must match the length Type::gadget. Lmk what you think. I'm happy to just remove the query_rand_len default impl if you think that's the best way

@divergentdave divergentdave merged commit a8e48e7 into divviup:main Nov 12, 2024
6 checks passed
@rozbb rozbb deleted the flp-compression branch November 12, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants