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

Remove use of Sage, reimplement field and polynomial operations #445

Merged
merged 17 commits into from
Oct 8, 2024

Conversation

divergentdave
Copy link
Collaborator

This is a first attempt at #431. It works, and it's slower, which is to be expected, but currently it's a lot slower. Unit tests take 22 seconds instead of 2 seconds. I think I may take a second pass at polynomial interpolation, using the method of divided differences, and see if that improves the speed somewhat.

@cjpatton
Copy link
Collaborator

cjpatton commented Oct 4, 2024

This is a first attempt at #431. It works, and it's slower, which is to be expected, but currently it's a lot slower. Unit tests take 22 seconds instead of 2 seconds. I think I may take a second pass at polynomial interpolation, using the method of divided differences, and see if that improves the speed somewhat.

22 is definitely too slow :) Another option is to just implement NTT? We only use it in FLP anyway.

@divergentdave
Copy link
Collaborator Author

Divided differences brings the time down to 10s. These are the remaining ideas I have for algorithmic speedups:

  • Conditionally use NTT if the interpolation points match
  • Use the extended Euclidean algorithm for multiplicative inverses, instead of Euler's theorem. Sage currently uses the extended Euclidean algorithm via GMP. (addition-chain exponentiation could speed up using Euler's theorem, but we don't care about constant timeness, and extended GCD is likely faster and simpler)
  • I recall discovering that Python's modular exponentiation algorithm for 3-argument pow() is slower than what Sage has, but that might have been only relevant for much larger inputs.

I'll do the extended Euclidean algorithm next, as that is easy to do, and we'll see how far that gets us.

By the time you're done, you will have written an ad hoc, informally-specified, bug-ridden, slow implementation of one percent of SageMath.

-- https://cryptopals.com/sets/8

@divergentdave divergentdave marked this pull request as ready for review October 7, 2024 21:42
@divergentdave
Copy link
Collaborator Author

Test suite runtime is now 6.7s in CI and 4.1s locally for me. I think this is about as good as we can get it without a lot more complexity.

@cjpatton
Copy link
Collaborator

cjpatton commented Oct 7, 2024

Test suite runtime is now 6.7s in CI and 4.1s locally for me. I think this is about as good as we can get it without a lot more complexity.

4 seconds is doable. Have you tried generating the test vectors? Hopefully that's quick, too.

My main concern about landing this is the runtime of PINE's (https://github.com/junyechen1996/draft-chen-cfrg-vdaf-pine/) unit tests, which at the moment take about 10 seconds. Let's prototype this in that repo before land this.

@divergentdave
Copy link
Collaborator Author

Test vector generation just takes 0.6s for me.

FWIW, here are the slowest tests. We could probably also speed things up by tweaking the dimensions they use, but I think that would be best handled separately.

Slowest test durations
----------------------------------------------------------------------
2.053s     test (tests.test_vdaf_prio3.TestPrio3SumVecWithMultiproof.test)
0.905s     test (tests.test_idpf_bbcggi21.TestIdpfBBCGGI21.test)
0.478s     test (tests.test_vdaf_prio3.TestPrio3SumVec.test)
0.219s     test_poplar1 (tests.test_vdaf_poplar1.TestPoplar1.test_poplar1)

@cjpatton
Copy link
Collaborator

cjpatton commented Oct 8, 2024

My main concern about landing this is the runtime of PINE's (https://github.com/junyechen1996/draft-chen-cfrg-vdaf-pine/) unit tests, which at the moment take about 10 seconds. Let's prototype this in that repo before land this.

I hacked this together here: junyechen1996/draft-chen-cfrg-vdaf-pine@bfe25cd

As expected, the unit tests take longer, about twice as long (now 12 seconds). Test vector generation also takes about twice as long, now 120 seconds. Note that we generate test vectors for higher dimensional data for PINE than we do for Prio3.

I still think we should take this change, given how much more portable it makes the reference code, but let's check with @junyechen1996 to make sure this isn't a deal breaker.

@divergentdave
Copy link
Collaborator Author

For PINE's sake, there's one other algorithmic improvement we may want to consider other than those listed above: multiplying polynomials using NTT instead of the straightforward quadratic algorithm. This only matters when multiplying large polynomials.

@cjpatton
Copy link
Collaborator

cjpatton commented Oct 8, 2024

For PINE's sake, there's one other algorithmic improvement we may want to consider other than those listed above: multiplying polynomials using NTT instead of the straightforward quadratic algorithm. This only matters when multiplying large polynomials.

Right, as we do in libprio already. How much would we have to change the spec? I wouldn't want to have to change too much to accommodate this. In particular, I wouldn't want to make the spec less clear.

@cjpatton
Copy link
Collaborator

cjpatton commented Oct 8, 2024

@junyechen1996 is cool with this change. I'll review tomorrow.

@cjpatton cjpatton self-requested a review October 8, 2024 01:01
@divergentdave
Copy link
Collaborator Author

Right, as we do in libprio already. How much would we have to change the spec? I wouldn't want to have to change too much to accommodate this. In particular, I wouldn't want to make the spec less clear.

Neither poly_mul() nor poly_interp() are excerpted in the spec, so this is all implementation details of the proof of concept.

@cjpatton
Copy link
Collaborator

cjpatton commented Oct 8, 2024

Right, as we do in libprio already. How much would we have to change the spec? I wouldn't want to have to change too much to accommodate this. In particular, I wouldn't want to make the spec less clear.

Neither poly_mul() nor poly_interp() are excerpted in the spec, so this is all implementation details of the proof of concept.

Got it. Let's keep it that way: if anyone tells us we need to list this, then we can do so.

As far as optimizing poly_mul(), I'm fine with holding off for now.

.github/workflows/test.yml Outdated Show resolved Hide resolved
@@ -14,8 +14,6 @@ draft-patton-cfrg-vdaf.xml
lib
report.xml
venv/
sagelib
*.sage.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Man, remember when we we're just had a bunch of .sage files? That was truly terrible. Thank you so much for your hard work on the software engineering for this spec.

poc/README.md Outdated Show resolved Hide resolved
poc/gen_test_vec.py Show resolved Hide resolved
poc/gen_test_vec.py Show resolved Hide resolved
poc/vdaf_poc/field.py Outdated Show resolved Hide resolved
poc/vdaf_poc/field.py Outdated Show resolved Hide resolved
poc/vdaf_poc/field.py Outdated Show resolved Hide resolved
poc/vdaf_poc/field.py Outdated Show resolved Hide resolved
poc/vdaf_poc/field.py Outdated Show resolved Hide resolved
@divergentdave divergentdave merged commit d8a6443 into main Oct 8, 2024
6 checks passed
@divergentdave divergentdave deleted the david/eject-sage branch October 8, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants