Rip out non-endomorphism code #826

pull sipa wants to merge 1 commits into bitcoin-core:master from sipa:202009_endo_endo_endo changing 17 files +20 −230

sipa commented at 3:07 AM on September 26, 2020: contributor

As the patent on the GLV optimization has expired, there is no need to keep the slower non-endomorphism code around anymore.
sipa force-pushed on Sep 26, 2020
sipa force-pushed on Sep 26, 2020
elichai commented at 8:51 AM on September 26, 2020: contributor

There's nothing I love more than a PR that only deletes code :)
sipa commented at 8:51 AM on September 26, 2020: contributor

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

Hey it also improves some comments.
in .travis.yml:42 in a523e2c996 outdated
```
  45 |    fast_finish: true
  46 |    include:
  47 |      - compiler: clang
  48 |        os: linux
  49 | -      env: HOST=i686-linux-gnu ENDOMORPHISM=yes
  50 | +      env: HOST=i686-linux-gnu
```
elichai commented at 8:55 AM on September 26, 2020:

This job can be completely removed, it's exactly the same as the one below

sipa commented at 8:57 AM on September 26, 2020:

One has gmp, the other doesn't. (same comment applies below).

elichai commented at 9:01 AM on September 26, 2020:

Oh you're right, my mistake.

in .travis.yml:66 in a523e2c996 outdated

  58 | @@ -63,7 +59,7 @@ matrix:
  59 |              - libtool-bin
  60 |              - libc6-dbg:i386
  61 |      - compiler: gcc
  62 | -      env: HOST=i686-linux-gnu ENDOMORPHISM=yes

elichai commented at 8:55 AM on September 26, 2020:

This job can be completely removed, it's exactly the same as the one below

elichai commented at 9:04 AM on September 26, 2020: contributor
tACK a523e2c9965ff43939abad555fcb8a8aed684d8e

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 commented at 3:21 PM on September 26, 2020: contributor

ACK a523e2c9965ff43939abad555fcb8a8aed684d8e (modulo merge conflict): diff looks correct
Rip out non-endomorphism code 46f9e0e745
sipa force-pushed on Sep 26, 2020
sipa commented at 6:14 PM on September 26, 2020: contributor
Rebased, and:
Can you also remove from here: https://github.com/bitcoin-core/secp256k1/blob/master/contrib/travis.sh#L16

Can you update the README to remove the "optionally":
Done.
gmaxwell commented at 9:08 PM on September 26, 2020: 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).
jonatack commented at 3:00 PM on September 28, 2020: member

46f9e0e745accde97742cb542d3c779f5b99b4a8 patch diff LGTM
tarcieri referenced this in commit 4858cb0b9d on Sep 28, 2020
tarcieri referenced this in commit 3ae2dd112c on Sep 28, 2020
practicalswift commented at 7:33 PM on September 28, 2020: contributor

re-ACK 46f9e0e745accde97742cb542d3c779f5b99b4a8
laanwj commented at 8:47 PM on September 28, 2020: member

ACK 46f9e0e745accde97742cb542d3c779f5b99b4a8

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)
elichai commented at 9:58 AM on September 29, 2020: contributor

re-ACK 46f9e0e745accde97742cb542d3c779f5b99b4a8
tarcieri referenced this in commit 0db4e7e945 on Sep 29, 2020
tarcieri referenced this in commit 2b8ab1c2cb on Sep 30, 2020
benthecarman commented at 4:07 PM on October 9, 2020: none

ACK 46f9e0e
sipa commented at 12:47 AM on October 10, 2020: contributor

I'd like #822 in before this, as should have all assurances we can get that the lambda-split values indeed stay in range.
sipa commented at 6:16 PM on October 11, 2020: contributor

Closing this in favor of #830, which has it rebased on #822 and #825.
sipa closed this on Oct 11, 2020
sipa referenced this in commit c6b6b8f1bb on Oct 14, 2020

Contributors

Linked (view graph)

#830 Rip out non-endomorphism code + dependencies