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: contributorAs 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: contributorThere’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:
- 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”:
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: contributorACK a523e2c9965ff43939abad555fcb8a8aed684d8e (modulo merge conflict): diff looks correct
-
sipa cross-referenced this on Sep 26, 2020 from issue Enable endomorphism by default by paulmillr
-
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: contributorLooks 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 cross-referenced this on Sep 28, 2020 from issue k256: enable endomorphism optimizations by default; remove non-endomorphism code by tarcieri
-
str4d cross-referenced this on Sep 28, 2020 from issue Use the endomorphism optimization for secp256k1 by str4d
-
jonatack commented at 3:00 pm on September 28, 2020: none46f9e0e745accde97742cb542d3c779f5b99b4a8 patch diff LGTM
-
tarcieri referenced this in commit 4858cb0b9d on Sep 28, 2020
-
tarcieri cross-referenced this on Sep 28, 2020 from issue k256: enable endomorphisms by default by tarcieri
-
tarcieri referenced this in commit 3ae2dd112c on Sep 28, 2020
-
practicalswift commented at 7:33 pm on September 28, 2020: contributorre-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) -
real-or-random cross-referenced this on Sep 29, 2020 from issue Enable GLV optimization? by oleiba
-
elichai commented at 9:58 am on September 29, 2020: contributorre-ACK 46f9e0e745accde97742cb542d3c779f5b99b4a8
-
tarcieri referenced this in commit 0db4e7e945 on Sep 29, 2020
-
tarcieri cross-referenced this on Sep 29, 2020 from issue k256: remove non-endomorphism code by tarcieri
-
tarcieri referenced this in commit 2b8ab1c2cb on Sep 30, 2020
-
sipa cross-referenced this on Oct 5, 2020 from issue Increase precision of g1 and g2. by roconnor-blockstream
-
benthecarman commented at 4:07 pm on October 9, 2020: noneACK 46f9e0e
-
sipa cross-referenced this on Oct 11, 2020 from issue Rip out non-endomorphism code + dependencies by sipa
-
sipa cross-referenced this on Oct 11, 2020 from issue Switch to our own memcmp function by real-or-random
-
sipa closed this on Oct 11, 2020
-
sipa referenced this in commit c6b6b8f1bb on Oct 14, 2020
-
azuchi cross-referenced this on Jan 6, 2022 from issue GLV optimization by default by azuchi