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

Implement generic 128-bit prime field arithmetic. #165

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Feb 1, 2022

Previous code considers only primes of at most 126 bits. Now, it adds support for 128-bit primes.

cc: @cjpatton

Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

I'm not qualified to speak to the correctness of the new algorithm, however the changes to the tests in fp.rs look functionally equivalent, and I'm comfortable on leaning on the existing test coverage and @armfazh's expertise here. I think this change is ready for review.

A couple high level notes:

  1. I think we ought to replace Field126 with Field128. The former isn't used for anything yet, and the only reason we had a 126-bit field is because that was the largest field we could support.
  2. The benchmarks show a performance regression resulting from the additional arithmetic, as much as 10% for some benchmarks. I think we can live with this regression as long as (a) @tgeoghegan et al find this doesn't over-burden prio-server (my guess is it won't, since we're talking about less than 10 microseconds additional time spent on proof verification) and (b) the regression would be mitigated by optimized or fiat-generated arithmetic (@armfazh can confirm whether this is the case).

@cjpatton cjpatton marked this pull request as ready for review February 2, 2022 02:41
@armfazh
Copy link
Contributor Author

armfazh commented Feb 2, 2022

Current code is generic about the prime size (bounded to 128 bits) and the prime shape.

On the one hand, we can use fiat-crypto to produce formally-verified implementation, but it requires to have a prime specified.

on the other hand, we can produce hand-crafted optimized code for primes that have a special shape. This has the advantage that code could run faster.

Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

I have enabled CI for @armfazh. I expect the build to fail since Field126 has been removed but there are references to this elsewhere. Please replace references to Field126 with Field128.

src/fp.rs Show resolved Hide resolved
@cjpatton cjpatton self-requested a review February 3, 2022 23:06
Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

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

I'm going to trust the both of you that the math here is correct. Further, we have a backward compatibility test with FieldPriov2 as used in ENPA, and that passed, so that gives me confidence that this change to the arithmetic doesn't break anything. Finally, changing Field126 to Field128 is risk-free because there's nothing deployed anywhere that used Field126 (if there is, and you're reading this, WHAT ARE YOU DOING).

As Chris said, I'm not really concerned with the potential perf regression here because it won't meaningfully affect existing deployments of libprio which are (1) heavily I/O bound and (2) have tons of idle time anyway.

So with all that in mind, I think this change is good to go.

@tgeoghegan tgeoghegan merged commit 108b45e into divviup:main Feb 7, 2022
@tgeoghegan
Copy link
Contributor

And thank you @armfazh for this contribution! All of us at ISRG are very excited to be collaborating with you on this work.

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.

3 participants