Rip out non-endomorphism code + dependencies #830

sipa · 2020-10-11T18:08:07Z

This is a rebased/combined version of the following pull requests/commits with minor changes:

Switch to our own memcmp function #825 Switch to our own memcmp function
- Modification: secp256k1_memcmp_var is marked static inline
- Modification: also replace memcmp with secp256k1_memcmp_var in exhaustive tests
- Modification: add reference to GCC bug 95189
Increase precision of g1 and g2. #822 Increase precision of g1 and g2
- Modification: use the new secp256k1_memcmp_var function instead of memcmp (see Increase precision of g1 and g2. #822 (comment))
- Modification: drop the " Allow secp256k1_split_lambda_verify to pass even in the presence of GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189." commit, as it's dealt with using secp256k1_memcmp_var.
- Modification: rename secp256k1_gej_mul_lambda -> secp256k1_ge_mul_lambda
A new commit that moves the lambda constant out of secp256k1_scalar_split_lambda and (_verify).
The test commit suggested here: Increase precision of g1 and g2. #822 (comment)
- Modification: use the new accessible secp256k1_const_lambda instead of duplicating it.
Rip out non-endomorphism code #826 Rip out non-endomorphism code
A new commit that reduces the size of the WNAF output to 129, as we now have proof that the split output is always 128 bits or less.
A new commit to more consistently use input:k, integer outputs:k1,k2, modulo n outputs:r1,r2

This allows us to shift by 256+128 = 384 bits, which is a multiple of the limb size of the scalar representation. This also happens to be the most precision possible for g2 that still fits into a 256-bit value.

gmaxwell · 2020-10-12T04:55:57Z

ACK

sipa · 2020-10-12T19:57:25Z

Comment by @real-or-random, putting it here so I don't forget:

here’s a nit: the comment for split_lambda is now confusingly far from the actual function. this bit me when looking at it today. the current order is:

huge comment

lambda constant

split_lambda_verify

split_lambda

we could make it

lambda constant

huge comment

split_lambda

split_lambda_verify

feel free to either fix or ignore

gmaxwell · 2020-10-12T20:04:25Z

putting verify last puts things out of calling order (split_lambda calls _verify). maybe instead just split up the comment and put the range validation proof stuff on the verify part.

src/scalar.h

src/scalar_impl.h

The VERIFY macro turns on various paranoid consistency checks, but the complete functionality should still be tested without it. This also adds a couple of static test points for extremely small split inputs/outputs. The existing bounds vectors already check extremely large outputs.

sipa · 2020-10-13T19:26:15Z

I reordered the comments and functions bit, and addressed @jonasnick' nits.

luke-jr · 2020-10-13T20:01:15Z

IMO if GCC bug 95189 is the only problem with normal memcmp, I don't think implementing a new one here is the right solution. At most, check for it in configure and add --no-builtin-memcmp ?

sipa · 2020-10-13T20:04:31Z

@luke-jr I tried writing a test for the bug in autoconfese, but it was more effort than it's worth I think, especially since none of the uses of memcmp in libsecp256k1 are performance critical. This approach also protects non-autoconf builds.

luke-jr · 2020-10-13T20:32:24Z

Moving memcmp discussion to #832

real-or-random · 2020-10-13T20:34:16Z

@luke-jr I tried writing a test for the bug in autoconfese, but it was more effort than it's worth I think, especially since none of the uses of memcmp in libsecp256k1 are performance critical. This approach also protects non-autoconf builds.

Agreed, just adding our memcmp is simpler.

@luke-jr Also see more discussion in #823 .

real-or-random · 2020-10-13T21:22:15Z

ACK c582aba code inspection, some tests, verified the new g1/g2 constants

(mod the decision how we want to solve #823 )

jonasnick

ACK c582aba didn't verify the proof

Summary: ``` This allows us to shift by 256+128 = 384 bits, which is a multiple of the limb size of the scalar representation. This also happens to be the most precision possible for g2 that still fits into a 256-bit value. ``` Partial backport 2/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@76ed922 Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8038

Summary: Partial backport 3/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@acab934 Depends on D8038. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8039

Summary: Partial backport 4/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@9aca2f7 Depends on D8039. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8040

Summary: Partial backport 5/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@9d2f2b4 Depends on D8040. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8041

Summary: Partial backport 6/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@fe7fc1f Depends on D8040. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8042

Summary: ``` The VERIFY macro turns on various paranoid consistency checks, but the complete functionality should still be tested without it. This also adds a couple of static test points for extremely small split inputs/outputs. The existing bounds vectors already check extremely large outputs. ``` Partial backport 7/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@ebad841 Depends on D8042. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8043

Summary: Partial backport 8/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@4232e5b Updates are necessary to accomodate our build system and Travis variations. Depends on D8043. Test Plan: ninja check-secp256k1 make check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8044

Summary: Partial backport 9/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@2edc514 Depends on D8044. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8045

Summary: Partial backport 10/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@63c6b71 Depends on D8045. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8046

Summary: Completes 11/11 backport of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@c582aba Depends on D8046. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8047

Summary: ``` This allows us to shift by 256+128 = 384 bits, which is a multiple of the limb size of the scalar representation. This also happens to be the most precision possible for g2 that still fits into a 256-bit value. ``` Partial backport 2/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@76ed922 Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8038

Summary: Partial backport 3/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@acab934 Depends on D8038. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8039

Summary: Partial backport 4/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@9aca2f7 Depends on D8039. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8040

Summary: Partial backport 5/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@9d2f2b4 Depends on D8040. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8041

Summary: Partial backport 6/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@fe7fc1f Depends on D8040. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8042

Summary: ``` The VERIFY macro turns on various paranoid consistency checks, but the complete functionality should still be tested without it. This also adds a couple of static test points for extremely small split inputs/outputs. The existing bounds vectors already check extremely large outputs. ``` Partial backport 7/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@ebad841 Depends on D8042. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8043

Summary: Partial backport 8/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@4232e5b Updates are necessary to accomodate our build system and Travis variations. Depends on D8043. Test Plan: ninja check-secp256k1 make check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8044

Summary: Partial backport 9/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@2edc514 Depends on D8044. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8045

Summary: Partial backport 10/11 of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@63c6b71 Depends on D8045. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8046

Summary: Completes 11/11 backport of secp256k1 [[bitcoin-core/secp256k1#830 | PR830]]: bitcoin-core/secp256k1@c582aba Depends on D8046. Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8047

real-or-random and others added 2 commits October 11, 2020 10:39

Switch to our own memcmp function

6173839

Fixes bitcoin-core#823.

Increase precision of g1 and g2

76ed922

This allows us to shift by 256+128 = 384 bits, which is a multiple of the limb size of the scalar representation. This also happens to be the most precision possible for g2 that still fits into a 256-bit value.

This was referenced Oct 11, 2020

Increase precision of g1 and g2. #822

Closed

Switch to our own memcmp function #825

Closed

Rip out non-endomorphism code #826

Closed

jonasnick reviewed Oct 13, 2020

View reviewed changes

src/scalar.h Outdated Show resolved Hide resolved

src/scalar_impl.h Outdated Show resolved Hide resolved

src/scalar_impl.h Show resolved Hide resolved

roconnor-blockstream and others added 7 commits October 13, 2020 11:31

Detailed comments for secp256k1_scalar_split_lambda

acab934

Add secp256k1_split_lambda_verify

9aca2f7

Add tests to exercise lambda split near bounds

9d2f2b4

Make lambda constant accessible

fe7fc1f

Rip out non-endomorphism code

4232e5b

WNAF of lambda_split output has max size 129

2edc514

sipa force-pushed the 202010_endo_omni branch 2 times, most recently from 41e3df2 to 3db2136 Compare October 13, 2020 19:22

sipa added 2 commits October 13, 2020 13:21

Reorder comments/function around scalar_split_lambda

63c6b71

Consistency improvements to the comments

c582aba

sipa force-pushed the 202010_endo_omni branch from 3db2136 to c582aba Compare October 13, 2020 20:22

jonasnick reviewed Oct 13, 2020

View reviewed changes

sipa merged commit c6b6b8f into bitcoin-core:master Oct 14, 2020

ofek mentioned this pull request Jan 19, 2021

upgrade libsecp256k1 ofek/coincurve#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rip out non-endomorphism code + dependencies #830

Rip out non-endomorphism code + dependencies #830

sipa commented Oct 11, 2020 •

edited

Loading

gmaxwell commented Oct 12, 2020

sipa commented Oct 12, 2020

gmaxwell commented Oct 12, 2020

sipa commented Oct 13, 2020

luke-jr commented Oct 13, 2020

sipa commented Oct 13, 2020 •

edited

Loading

luke-jr commented Oct 13, 2020

real-or-random commented Oct 13, 2020 •

edited

Loading

real-or-random commented Oct 13, 2020 •

edited

Loading

jonasnick left a comment •

edited

Loading

Rip out non-endomorphism code + dependencies #830

Rip out non-endomorphism code + dependencies #830

Conversation

sipa commented Oct 11, 2020 • edited Loading

gmaxwell commented Oct 12, 2020

sipa commented Oct 12, 2020

gmaxwell commented Oct 12, 2020

sipa commented Oct 13, 2020

luke-jr commented Oct 13, 2020

sipa commented Oct 13, 2020 • edited Loading

luke-jr commented Oct 13, 2020

real-or-random commented Oct 13, 2020 • edited Loading

real-or-random commented Oct 13, 2020 • edited Loading

jonasnick left a comment • edited Loading

Choose a reason for hiding this comment

sipa commented Oct 11, 2020 •

edited

Loading

sipa commented Oct 13, 2020 •

edited

Loading

real-or-random commented Oct 13, 2020 •

edited

Loading

real-or-random commented Oct 13, 2020 •

edited

Loading

jonasnick left a comment •

edited

Loading