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.

Sign up for GitHub

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

Jump to bottom

Rip out non-endomorphism code #826

Closed
wants to merge 1 commit into from
Closed

Rip out non-endomorphism code #826

wants to merge 1 commit into from

Conversation

sipa
Copy link
Contributor

@sipa sipa commented Sep 26, 2020 •
edited
Loading

As the patent on the GLV optimization has expired, there is no need to keep the slower non-endomorphism code around anymore.

@sipa sipa force-pushed the 202009_endo_endo_endo branch 2 times, most recently from e199824 to a523e2c Compare September 26, 2020 03:47
@elichai
Copy link
Contributor

elichai commented Sep 26, 2020

There's nothing I love more than a PR that only deletes code :)

@sipa
Copy link
Contributor Author

sipa commented Sep 26, 2020

There's nothing I love more than a PR that only deletes code :)

Hey it also improves some comments.

elichai
elichai reviewed Sep 26, 2020
View reviewed changes
.travis.yml Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@elichai
Copy link
Contributor

elichai commented Sep 26, 2020

tACK a523e2c

two nits:

  1. Can you also remove from here: https://github.com/bitcoin-core/secp256k1/blob/master/contrib/travis.sh#L16
  2. Can you update the README to remove the "optionally":

Optionally (off by default) use secp256k1's efficiently-computable endomorphism to split the P multiplicand into 2 half-sized ones.

@practicalswift
Copy link
Contributor

ACK a523e2c (modulo merge conflict): diff looks correct

@sipa sipa mentioned this pull request Sep 26, 2020
Closed
@sipa sipa force-pushed the 202009_endo_endo_endo branch from a523e2c to 46f9e0e Compare September 26, 2020 18:13
@sipa
Copy link
Contributor Author

sipa commented Sep 26, 2020

Rebased, and:

  1. Can you also remove from here: https://github.com/bitcoin-core/secp256k1/blob/master/contrib/travis.sh#L16
  2. Can you update the README to remove the "optionally":

Done.

@gmaxwell
Copy link
Contributor

Looks good to me, endo only increases a stripped Os build for me by 456 bytes-- so insignificant esp with respect to the performance difference, so I don't see any reason to keep the other code around even for code size constrained applications (you can get 456 bytes of savings in other ways with less performance impact).

@tarcieri tarcieri mentioned this pull request Sep 28, 2020
Closed
@str4d str4d mentioned this pull request Sep 28, 2020
Merged
jonatack
jonatack reviewed Sep 28, 2020
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

46f9e0e patch diff LGTM

tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Sep 28, 2020
@tarcieri
k256: enable endomorphisms by default
4858cb0 
The reason this wasn't enabled-by-default before was due to a lingering
US patent. However, as noted on #211, that patent just expired 🎉

The `bitcoin-core/secp256k1` library is also pursuing a full switch to
endomorphisms: bitcoin-core/secp256k1#826

For now this commit just switches on the feature by default, but in the
future we can eventually rip out the non-endomorphism code and remove
the feature entirely.
@tarcieri tarcieri mentioned this pull request Sep 28, 2020
Merged
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Sep 28, 2020
@tarcieri
k256: enable endomorphisms by default (#213)
3ae2dd1 
The reason this wasn't enabled-by-default before was due to a lingering
US patent. However, as noted on #211, that patent just expired 🎉

The `bitcoin-core/secp256k1` library is also pursuing a full switch to
endomorphisms: bitcoin-core/secp256k1#826

For now this commit just switches on the feature by default, but in the
future we can eventually rip out the non-endomorphism code and remove
the feature entirely.
@practicalswift
Copy link
Contributor

re-ACK 46f9e0e

@laanwj
Copy link
Member

laanwj commented Sep 28, 2020 •
edited
Loading

ACK 46f9e0e

Looks good to me, endo only increases a stripped Os build for me by 456 bytes-- so insignificant esp with respect to the performance difference

I agree. I also looked at it from that angle, but this is a negligible increase in code size. Not worth keeping an option around for.
(on RV64 there is no size difference for the stripped libsecp256k1.so.0.0.0 between master and master with this patch merged)

@real-or-random real-or-random mentioned this pull request Sep 29, 2020
Closed
@elichai
Copy link
Contributor

elichai commented Sep 29, 2020

re-ACK 46f9e0e

tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Sep 29, 2020
@tarcieri
k256: remove non-endomorphism code
0db4e7e 
This code was previously gated under the `endomorphism-mul` feature due
to lingering concerns about the "GLV" patents, namely US7110538.

According to @pwuille, a patent attorney he works with has verified that
the patent is expired:

https://twitter.com/pwuille/status/1310639051393830912

Likewise non-endomorphism code is being removed from
`bitcoin-core/secp256k1`:

bitcoin-core/secp256k1#826

This commit does the same for `k256`: it removes all the
non-endomorphism code and feature gating from the endomorphism code,
making the `endomorphism-mul` feature a no-op.

We can remove the feature in the next SemVer-breaking release (v0.6.x)
@tarcieri tarcieri mentioned this pull request Sep 29, 2020
Merged
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Sep 30, 2020
@tarcieri
k256: remove non-endomorphism code (#217)
2b8ab1c 
This code was previously gated under the `endomorphism-mul` feature due
to lingering concerns about the "GLV" patents, namely US7110538.

According to @pwuille, a patent attorney he works with has verified that
the patent is expired:

https://twitter.com/pwuille/status/1310639051393830912

Likewise non-endomorphism code is being removed from
`bitcoin-core/secp256k1`:

bitcoin-core/secp256k1#826

This commit does the same for `k256`: it removes all the
non-endomorphism code and feature gating from the endomorphism code,
making the `endomorphism-mul` feature a no-op.

We can remove the feature in the next SemVer-breaking release (v0.6.x)
@sipa sipa mentioned this pull request Oct 5, 2020
Closed
@benthecarman
Copy link

ACK 46f9e0e

@sipa
Copy link
Contributor Author

sipa commented Oct 10, 2020 •
edited
Loading

I'd like #822 in before this, as should have all assurances we can get that the lambda-split values indeed stay in range.

This was referenced Oct 11, 2020
Merged
Closed
@sipa
Copy link
Contributor Author

sipa commented Oct 11, 2020

Closing this in favor of #830, which has it rebased on #822 and #825.

@sipa sipa closed this Oct 11, 2020
sipa added a commit that referenced this pull request Oct 14, 2020
@sipa
Merge #830: Rip out non-endomorphism code + dependencies
c6b6b8f 
c582aba Consistency improvements to the comments (Pieter Wuille)
63c6b71 Reorder comments/function around scalar_split_lambda (Pieter Wuille)
2edc514 WNAF of lambda_split output has max size 129 (Pieter Wuille)
4232e5b Rip out non-endomorphism code (Pieter Wuille)
ebad841 Check correctness of lambda split without -DVERIFY (Gregory Maxwell)
fe7fc1f Make lambda constant accessible (Pieter Wuille)
9d2f2b4 Add tests to exercise lambda split near bounds (Pieter Wuille)
9aca2f7 Add secp256k1_split_lambda_verify (Russell O'Connor)
acab934 Detailed comments for secp256k1_scalar_split_lambda (Russell O'Connor)
76ed922 Increase precision of g1 and g2 (Russell O'Connor)
6173839 Switch to our own memcmp function (Tim Ruffing)

Pull request description:

  This is a rebased/combined version of the following pull requests/commits with minor changes:
  * #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
  * #822 Increase precision of g1 and g2
    * Modification: use the new `secp256k1_memcmp_var` function instead of `memcmp` (see #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: #822 (comment)
    * Modification: use the new accessible `secp256k1_const_lambda` instead of duplicating it.
  * #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`

ACKs for top commit:
  real-or-random:
    ACK c582aba code inspection, some tests, verified the new g1/g2 constants
  jonasnick:
    ACK c582aba didn't verify the proof

Tree-SHA512: 323a3ee3884b7ac4fa85c8e7b785111b5c0638d718bc1c805a38963c87411e81a746c98e9a42a3e2197ab34a874544de5cc51326955d1c4d0ea45afd418e819f
@azuchi azuchi mentioned this pull request Jan 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonatack jonatack jonatack left review comments

@elichai elichai elichai left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sipa @elichai @practicalswift @gmaxwell @laanwj @benthecarman @jonatack