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

Update libsecp256k1 (endomorphism, test improvements) #20147

Merged
merged 2 commits into from Oct 15, 2020

Conversation

@sipa
Copy link
Member

@sipa sipa commented Oct 14, 2020

This updates the libsecp256k1 subtree to the latest master, which includes:

  • Enabling the GLV endomorphism optimization by default (and removing support for the non-GLV EC multiplication)
  • Added a proof for the correctness of the lambda split algorithm by roconnor-blockstream (other code was relying on the fact that it always outputs 128 bit results, which isn't at all obvious).
  • Improved exhaustive tests, in particular for the Schnorr signature module
  • Various other testing and CI improvements
sipa added 2 commits Oct 14, 2020
c6b6b8f1bb Merge bitcoin#830: Rip out non-endomorphism code + dependencies
c582abade1 Consistency improvements to the comments
63c6b71616 Reorder comments/function around scalar_split_lambda
2edc514c90 WNAF of lambda_split output has max size 129
4232e5b7da Rip out non-endomorphism code
ebad8414b0 Check correctness of lambda split without -DVERIFY
fe7fc1fda8 Make lambda constant accessible
9d2f2b44d8 Add tests to exercise lambda split near bounds
9aca2f7f07 Add secp256k1_split_lambda_verify
acab934d24 Detailed comments for secp256k1_scalar_split_lambda
76ed922a5f Increase precision of g1 and g2
6173839c90 Switch to our own memcmp function
63150ab4da Merge bitcoin#827: Rename testrand functions to have test in name
c5257aed0b Merge bitcoin#821: travis: Explicitly set --with-valgrind
bb1f54280f Merge bitcoin#818: Add static assertion that uint32_t is unsigned int or wider
a45c1fa63c Rename testrand functions to have test in name
5006895bd6 Merge bitcoin#808: Exhaustive test improvements + exhaustive schnorrsig tests
4eecb4d6ef travis: VALGRIND->RUN_VALGRIND to avoid confusion with WITH_VALGRIND
66a765c775 travis: Explicitly set --with-valgrind
d7838ba6a6 Merge bitcoin#813: Enable configuring Valgrind support
7ceb0b7611 Merge bitcoin#819: Enable -Wundef warning
8b7dcdd955 Add exhaustive test for extrakeys and schnorrsig
08d7d89299 Make pubkey parsing test whether points are in the correct subgroup
87af00b511 Abstract out challenge computation in schnorrsig
63e1b2aa7d Disable output buffering in tests_exhaustive.c
39f67dd072 Support splitting exhaustive tests across cores
e99b26fcd5 Give exhaustive_tests count and seed cmdline inputs
49e6630bca refactor: move RNG seeding to testrand
b110c106fa Change exhaustive test groups so they have a point with X=1
cec7b18a34 Select exhaustive lambda in function of order
78f6cdfaae Make the curve B constant a secp256k1_fe
d7f39ae4b6 Delete gej_is_valid_var: unused outside tests
8bcd78cd79 Make secp256k1_scalar_b32 detect overflow in scalar_low
c498366e5b Move exhaustive tests for recovery to module
be31791543 Make group order purely compile-time in exhaustive tests
e73ff30922 Enable -Wundef warning
c0041b5cfc Add static assertion that uint32_t is unsigned int or wider
4ad408faf3 Merge bitcoin#782: Check if variable=yes instead of if var is set in travis.sh
412bf874d0 configure: Allow specifying --with[out]-valgrind explicitly
34debf7a6d Modify .travis.yml to explictly pass no in env vars instead of setting to nothing
a0e99fc121 Merge bitcoin#814: tests: Initialize random group elements fully
5738e8622d tests: Initialize random group elements fully
c9939ba55d Merge bitcoin#812: travis: run bench_schnorrsig
a51f2af62b travis: run bench_schnorrsig
ef37761fee Change travis.sh to check if variables are equal to yes instead of not-empty. Before this, setting `VALGRIND=wat` was considered as true, and to make it evaluate as false you had to unset the variable `VALGRIND=` but not it checks if `VALGRIND=yes` and if it's not `yes` then it's evaluated to false

git-subtree-dir: src/secp256k1
git-subtree-split: c6b6b8f1bb044d7d1aa065ebb674adde98a36a8e
@jonatack
Copy link
Member

@jonatack jonatack commented Oct 14, 2020

Concept ACK.

Loading

@benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Oct 14, 2020

ACK 9e5626d

$  ./test/lint/git-subtree-check.sh src/secp256k1
src/secp256k1 in HEAD currently refers to tree c79797189781d6656eba6cb155c249c57b4f5c9a
src/secp256k1 in HEAD was last updated in commit 52380bf304b1c02dda23f1e2fad0159e29b2f7a2 (tree c79797189781d6656eba6cb155c249c57b4f5c9a)
src/secp256k1 in HEAD was last updated to upstream commit c6b6b8f1bb044d7d1aa065ebb674adde98a36a8e (tree c79797189781d6656eba6cb155c249c57b4f5c9a)
GOOD

Loading

Copy link
Member

@fanquake fanquake left a comment

ACK 9e5626d - performed a squash and checked that the changes were the same. The non-endomorphism code has now been ripped out.

how-to-verify-subtree notes here: https://github.com/fanquake/core-review/blob/master/subtree-merge.md

Loading

@fanquake
Copy link
Member

@fanquake fanquake commented Oct 15, 2020

@jamesob this would be a good PR for you to throw into bitcoinperf.

Loading

@fanquake fanquake merged commit f2e6d14 into bitcoin:master Oct 15, 2020
5 checks passed
Loading
@MarcoFalke MarcoFalke removed the Docs label Oct 15, 2020
@jamesob
Copy link
Member

@jamesob jamesob commented Oct 16, 2020

Ran a local IBD comparison on this for 8000 blocks (height 500_000 - 508_000) and to my surprise didn't see any performance difference. Compared the HEAD of this PR to the commit in master preceding it (c2c4dba). Could be that this platform is bottlenecked by IO and not CPU?

ibd local range 500000 508000

commands index

bench name command
ibd.local.range.500000.508000 bitcoind -dbcache=300 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 -assumevalid=000000000000000000176c192f42ad13ab159fdb20198b87e7ba3c001e47b876

#20147 vs. c2c4dba (absolute)

bench name x #20147 c2c4dba
ibd.local.range.500000.508000.total_secs 2 737.8656 (± 3.1841) 738.3541 (± 7.6443)
ibd.local.range.500000.508000.peak_rss_KiB 2 1746294.0000 (± 2362.0000) 1756260.0000 (± 3168.0000)

#20147 vs. c2c4dba (relative)

bench name x #20147 c2c4dba
ibd.local.range.500000.508000.total_secs 2 1 1.001
ibd.local.range.500000.508000.peak_rss_KiB 2 1 1.006

Loading

@sipa
Copy link
Member Author

@sipa sipa commented Oct 16, 2020

@jamesob That's certainly possible. With such a low dbcache, it's also possible it's just spending a lot of time in in preparing leveldb updates to flush.

Loading

sidhujag added a commit to syscoin/syscoin that referenced this issue Oct 16, 2020
…ents)

52380bf Squashed 'src/secp256k1/' changes from 8ab24e8dad..c6b6b8f1bb (Pieter Wuille)

Pull request description:

  This updates the libsecp256k1 subtree to the latest master, which includes:

  * Enabling the GLV endomorphism optimization by default (and removing support for the non-GLV EC multiplication)
  * Added a proof for the correctness of the lambda split algorithm by roconnor-blockstream (other code was relying on the fact that it always outputs 128 bit results, which isn't at all obvious).
  * Improved exhaustive tests, in particular for the Schnorr signature module
  * Various other testing and CI improvements

ACKs for top commit:
  fanquake:
    ACK 9e5626d - performed a squash and checked that the changes were the same. The non-endomorphism code has now been ripped out.
  benthecarman:
    ACK 9e5626d

Tree-SHA512: 50fda5f3f934ee525f01cfc15e4f5efbc5261a97f2b77fe1b3453ee0edcf1281ad74ab4532a2fe1fe907652dd47023beff8cf3d73bf34f65ac914a694b9e7110
@Sjors
Copy link
Member

@Sjors Sjors commented Oct 16, 2020

Your assumevalid block is at height 522,000 so you're not checking signatures, I think.

Loading

@jamesob
Copy link
Member

@jamesob jamesob commented Oct 17, 2020

Great call @Sjors, that should've occurred to me (d'oh). In any case, here are some much less surprising results showing improvement after disabled assumevalid.

ibd local range 500000 508000

commands index

bench name command
ibd.local.range.500000.508000 bitcoind -dbcache=300 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 \-assumevalid=0

#20147 vs. c2c4dba (absolute)

bench name x #20147 c2c4dba
ibd.local.range.500000.508000.total_secs 2 1529.8273 (± 5.1097) 1780.5817 (± 5.7187)
ibd.local.range.500000.508000.peak_rss_KiB 2 1718228.0000 (± 17852.0000) 1686864.0000 (± 6596.0000)

#20147 vs. c2c4dba (relative)

bench name x #20147 c2c4dba
ibd.local.range.500000.508000.total_secs 2 1.000 1.164
ibd.local.range.500000.508000.peak_rss_KiB 2 1.019 1.000

For kicks, here's the utxo cache flush history (I need to normalize the times in this graph...).

ibd local range 500000 508000-flush-history

Loading

@fanquake
Copy link
Member

@fanquake fanquake commented Oct 19, 2020

Thanks @jamesob 👍

Loading

zkbot added a commit to zcash/zcash that referenced this issue Oct 24, 2020
Update secp256k1 again

This migrates us to the same dependency version that upstream Bitcoin
Core migrated to in bitcoin/bitcoin#20147.
zkbot added a commit to zcash/zcash that referenced this issue Oct 25, 2020
Update secp256k1 again

This migrates us to the same dependency version that upstream Bitcoin
Core migrated to in bitcoin/bitcoin#20147.
UdjinM6 added a commit to UdjinM6/dash that referenced this issue Aug 10, 2021
…ents)

52380bf Squashed 'src/secp256k1/' changes from 8ab24e8dad..c6b6b8f1bb (Pieter Wuille)

Pull request description:

  This updates the libsecp256k1 subtree to the latest master, which includes:

  * Enabling the GLV endomorphism optimization by default (and removing support for the non-GLV EC multiplication)
  * Added a proof for the correctness of the lambda split algorithm by roconnor-blockstream (other code was relying on the fact that it always outputs 128 bit results, which isn't at all obvious).
  * Improved exhaustive tests, in particular for the Schnorr signature module
  * Various other testing and CI improvements

ACKs for top commit:
  fanquake:
    ACK 9e5626d - performed a squash and checked that the changes were the same. The non-endomorphism code has now been ripped out.
  benthecarman:
    ACK 9e5626d

Tree-SHA512: 50fda5f3f934ee525f01cfc15e4f5efbc5261a97f2b77fe1b3453ee0edcf1281ad74ab4532a2fe1fe907652dd47023beff8cf3d73bf34f65ac914a694b9e7110
5tefan added a commit to 5tefan/dash that referenced this issue Aug 12, 2021
…ents)

52380bf Squashed 'src/secp256k1/' changes from 8ab24e8dad..c6b6b8f1bb (Pieter Wuille)

Pull request description:

  This updates the libsecp256k1 subtree to the latest master, which includes:

  * Enabling the GLV endomorphism optimization by default (and removing support for the non-GLV EC multiplication)
  * Added a proof for the correctness of the lambda split algorithm by roconnor-blockstream (other code was relying on the fact that it always outputs 128 bit results, which isn't at all obvious).
  * Improved exhaustive tests, in particular for the Schnorr signature module
  * Various other testing and CI improvements

ACKs for top commit:
  fanquake:
    ACK 9e5626d - performed a squash and checked that the changes were the same. The non-endomorphism code has now been ripped out.
  benthecarman:
    ACK 9e5626d

Tree-SHA512: 50fda5f3f934ee525f01cfc15e4f5efbc5261a97f2b77fe1b3453ee0edcf1281ad74ab4532a2fe1fe907652dd47023beff8cf3d73bf34f65ac914a694b9e7110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants