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

shallue_van_de_woestijne rewrite #286

Merged

Conversation

roconnor-blockstream
Copy link
Contributor

The previous implementation returns an off-curve point for the input t=0.

This rewrite addresses that issue by implicity returning the on-curve point (d, sqrt(8)), which is the point that the paper Indifferentiable Hashing to Barreto–Naehrig Curves suggests returning in this case.

Note: At the moment it is cryptographically impossible for the input t to be 0.

@roconnor-blockstream
Copy link
Contributor Author

This is a third alternative to #284 and #283. It is functionally the same as #284, but is a more invasive change to the code base.

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 421d39d stepped through each line of the algebra and confirmed it matches the (unchanged) summary comment; did not carefully check magnitudes

@roconnor-blockstream
Copy link
Contributor Author

I didn't actually count the operations. I just eyeballed that this was better.

AFAICT, SECP256K1_GE_X_MAGNITUDE_MAX is 4, which this implementation does satisfy.

@apoelstra
Copy link
Contributor

A quick count shows you are deleting 7 muls+sqrs and adding 6. The number of invs stays at 1. So it's actually pretty close, but I think this is an improvement.

@roconnor-blockstream
Copy link
Contributor Author

That seems off to me.

I just counted deleting 11 muls+sqrs and adding 5 (I assert that 'mul_int' doesn't count as a mul).

@apoelstra
Copy link
Contributor

Oh, you're right, I remembered not to count mul_int against you but then also forgot to count the original muls. So I meant to say "deleting 10 and adding 6". Furthermore, I may have miscounted...

A more careful count:

  • -9 mul, +4 mul
  • -2 sqr, +1 sqr

Which matches your numbers.

@roconnor-blockstream
Copy link
Contributor Author

Just a note for reviewers that this "speed improvement" is simply incidental. Given the enormous number of square root operations the function has, it is not expected to make a serious impact on execution time.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Concept ACK

I wrote a commit that adds a test for t = 0 (which confirms the off-curve point on master) and documentation improvements: https://github.com/jonasnick/secp256k1-zkp/commits/2024-01-swu-rewrite-jn/

Feel free to cherry-pick.

I also checked algebra & magnitudes.

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 26ab228

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK mod nits

src/modules/generator/main_impl.h Outdated Show resolved Hide resolved
@@ -48,7 +48,9 @@ static void test_generator_api(void) {

static void test_shallue_van_de_woestijne(void) {
/* Matches with the output of the shallue_van_de_woestijne.sage SAGE program */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to change the loop there to cover the new points? The script could also print out -c and d.

"No" is fine as an answer. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did look into this, but I couldn't figure out an elegant way to change the sage code myself, being neither a sage nor a python expert. I'd welcome such a patch though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous implementation returns an off-curve point for the input t=0.

This rewrite addresses that issue by implicity returning the on-curve point
(d, sqrt(1 + b)), which is the point that the paper Indifferentiable Hashing
to Barreto–Naehrig Curves suggests returning in this case.

Note: At the moment it is cryptographically impossible for the input t to be 0.
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 6b9d335

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK 6b9d335

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.

None yet

4 participants