Skip to content

Conversation

sipa
Copy link
Member

@sipa sipa commented Dec 13, 2022

Now that libsecp256k1 has a release (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-December/021271.html), update the subtree to match it.

The changes themselves are not very impactful for Bitcoin Core, but include:

  • It's no longer needed to specify whether contexts are for signing or verification or both (all contexts support everything), so make use of that in this PR.
  • Verification operations can use the static context now, removing the need for some infrastructure in pubkey.cpp to make sure a context exists.
  • Most modules are now enabled by default, so we can drop explicit enabling for them.
  • CI improvements (in particular, MSVC and more recent MacOS)
  • Introduction of an internal int128 type, which has no effect for GCC/Clang builds, but enables 128-bit multiplication in MSVC, giving a ~20% speedup there (but still slower than GCC/Clang).
  • Release process changes (process documentation, changelog, ...).

sipa added 2 commits December 12, 2022 23:40
21ffe4b Merge bitcoin-core/secp256k1#1055: Prepare initial release
e025ccd release: prepare for initial release 0.2.0
6d1784a build: add missing files to EXTRA_DIST
8c949f5 Merge bitcoin-core/secp256k1#1173: Don't use compute credits for now
13bf1b6 changelog: make order of change types match keepachangelog.com
b1f992a doc: improve release process
7e5b226 Don't use compute credits for now
ad39e2d build: change package version to 0.1.0-dev
5c789dc Merge bitcoin-core/secp256k1#1168: Replace deprecated context flags with NONE in benchmarks and tests
d6dc0f4 tests: Switch to NONE contexts in module tests
0c8a5ca tests: Switch to NONE contexts in tests.c
86540e9 tests: add test for deprecated flags and rm them from run_context
caa0ad6 group: add gej_eq_var
37ba744 tests: Switch to NONE contexts in exhaustive and ctime tests
8d7a9a8 benchmarks: Switch to NONE contexts
90618e9 doc: move CHANGELOG from doc/ to root directory
e3f8477 Merge bitcoin-core/secp256k1#1126: API cleanup with respect to contexts
4386a23 examples: Switch to NONE contexts
7289b51 docs: Use doxygen style if and only if comment is user-facing
e7d0185 docs: Get rid of "initialized for signing" terminology
0612636 docs: Tidy and improve docs about contexts and randomization
e02d686 selftest: Expose in public API
e383fbf selftest: Rename internal function to make name available for API
d2c6d48 tests: Use new name of static context
53796d2 contexts: Rename static context
72fedf8 docs: Improve docs for static context
316ac76 contexts: Deprecate all context flags except SECP256K1_CONTEXT_NONE
477f02c Merge bitcoin-core/secp256k1#1165: gitignore: Add *.sage.py files autogenerated by sage [skip ci]
092be61 gitignore: Add *.sage.py files autogenerated by sage
1a553ee docs: Change signature "validation" to "verification"
ee7341f docs: Never require a verification context
751c435 Merge bitcoin-core/secp256k1#1152: Update macOS image for CI
2286f80 Merge bitcoin-core/secp256k1#993: Enable non-experimental modules by default
e40fd27 Merge bitcoin-core/secp256k1#1156: Followups to int128_struct arithmetic
99bd335 Make int128 overflow test use secp256k1_[ui]128_mul
a8494b0 Use compute credits for macOS jobs
3afce0a Avoid signed overflow in MSVC AMR64 secp256k1_mul128
c0ae48c Update macOS image for CI
9b5f589 Heuristically decide whether to use int128_struct
63ff064 int128: Add test override for testing __(u)mulh on MSVC X64
f2b7e88 Add int128 randomized tests
6138d73 Merge bitcoin-core/secp256k1#1155: Add MSan CI jobs
ddf2b29 Merge bitcoin-core/secp256k1#1000: Synthetic int128 type.
86e3b38 Merge bitcoin-core/secp256k1#1149: Remove usage of CHECK from non-test file
00a42b9 Add MSan CI job
44916ae Merge bitcoin-core/secp256k1#1147: ci: print env to allow reproducing the job outside of CI
c2ee917 Merge bitcoin-core/secp256k1#1146: ci: prevent "-v/--version: not found" irrelevant error
e13fae4 Merge bitcoin-core/secp256k1#1150: ci: always cat test_env.log
a340d95 ci: add int128_struct tests
dceaa1f int128: Tidy #includes of int128.h and int128_impl.h
2914bcc Simulated int128 type.
6a965b6 Remove usage of CHECK from non-test file
5c9f1a5 ci: always cat all logs_snippets
49ae843 ci: mostly prevent "-v/--version: not found" irrelevant error
4e54c03 ci: print env to allow reproducing the job outside of CI
a43e982 Merge bitcoin-core/secp256k1#1144: Cleanup `.gitignore` file
f5039cb Cleanup `.gitignore` file
798727a Revert "Add test logs to gitignore"
41e8704 build: Enable some modules by default
694ce8f Merge bitcoin-core/secp256k1#1131: readme: Misc improvements
88b0089 readme: Fix line break
78f5296 readme: Sell "no runtime dependencies"
ef48f08 readme: Add IRC channel
9f8a13d Merge bitcoin-core/secp256k1#1128: configure: Remove pkgconfig macros again (reintroduced by mismerge)
cabe085 configure: Remove pkgconfig macros again (reintroduced by mismerge)
3efeb9d Merge bitcoin-core/secp256k1#1121: config: Set preprocessor defaults for ECMULT_* config values
6a873cc Merge bitcoin-core/secp256k1#1122: tests: Randomize the context with probability 15/16 instead of 1/4
17065f4 tests: Randomize the context with probability 15/16 instead of 1/4
c27ae45 config: Remove basic-config.h
da6514a config: Introduce DEBUG_CONFIG macro for debug output of config
63a3565 Merge bitcoin-core/secp256k1#1120: ecmult_gen: Skip RNG when creating blinding if no seed is available
d0cf55e config: Set preprocessor defaults for ECMULT_* config values
55f8bc9 ecmult_gen: Improve comments about projective blinding
7a86955 ecmult_gen: Simplify code (no observable change)
4cc0b1b ecmult_gen: Skip RNG when creating blinding if no seed is available
af65d30 Merge bitcoin-core/secp256k1#1116: build: Fix #include "..." paths to get rid of further -I arguments
40a3473 build: Fix #include "..." paths to get rid of further -I arguments
43756da Merge bitcoin-core/secp256k1#1115: Fix sepc256k1 -> secp256k1 typo in group.h
069aba8 Fix sepc256k1 -> secp256k1 typo in group.h
accadc9 Merge bitcoin-core/secp256k1#1114: `_scratch_destroy`: move `VERIFY_CHECK` after invalid scrach space check
cd47033 Merge bitcoin-core/secp256k1#1084: ci: Add MSVC builds
1827c9b scratch_destroy: move VERIFY_CHECK after invalid scrach space check
49e2acd configure: Improve rationale for WERROR_CFLAGS
8dc4b03 ci: Add a C++ job that compiles the public headers without -fpermissive
51f296a ci: Run persistent wineserver to speed up wine
3fb3269 ci: Add 32-bit MinGW64 build
9efc2e5 ci: Add MSVC builds
2be6ba0 configure: Convince autotools to work with MSVC's archiver lib.exe
bd81f41 schnorrsig bench: Suppress a stupid warning in MSVC
09f3d71 configure: Add a few CFLAGS for MSVC
3b4f3d0 build: Reject C++ compilers in the preprocessor
1cc0941 configure: Don't abort if the compiler does not define __STDC__
cca8cbb configure: Output message when checking for valgrind
1a6be57 bench: Make benchmarks compile on MSVC

git-subtree-dir: src/secp256k1
git-subtree-split: 21ffe4b
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 13, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK Sjors, achow101, jonasnick
Concept ACK real-or-random

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25465 (build: remove boost library detection by fanquake)
  • #24742 (build: prune Boost headers in depends by fanquake)
  • #23561 (BIP324: Handshake prerequisites by dhruv)
  • #23432 (BIP324: CPubKey encode/decode to elligator-swift by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Concept ACK

@real-or-random
Copy link
Contributor

real-or-random commented Dec 13, 2022

The third commit says "Invoke secp256k1_selftest() during initialization." but that happens in a different commit.

edit: by the way, if we can guarantee that the secp256k1_context_create(SECP256k1_CONTEXT_NONE) in key.cpp runs before the functions in pubkey.cpp, we could in theory drop the secp256k1_selftest() call. But yeah, I don't think it's a good idea: it creates more coupling between the files and is easy to overlook in the future.

sipa added 3 commits December 13, 2022 15:08
* Use SECP256K1_CONTEXT_NONE when creating signing context, as
  SECP256K1_CONTEXT_SIGN is deprecated and unnecessary.
* Use secp256k1_static_context where applicable.
@sipa
Copy link
Member Author

sipa commented Dec 13, 2022

The third commit says "Invoke secp256k1_selftest() during initialization." but that happens in a different commit.

Fixed.

@bitcoin bitcoin deleted a comment from DrahtBot Dec 13, 2022
@Sjors
Copy link
Member

Sjors commented Dec 13, 2022

I compiled 2022917 on a Linux and an Intel mac, compiled and successfully ran the tests.

@DrahtBot
Copy link
Contributor

Guix builds

File commit 678889e
(master)
commit 2ea8f89
(master and this pull)
SHA256SUMS.part 49aa40ef9b94f7ce... c1dcb3f424a2e99e...
*-aarch64-linux-gnu-debug.tar.gz a93340960b19c90a... ecb64fc6bb49c24c...
*-aarch64-linux-gnu.tar.gz f7d9b398bec8ed3a... 95b5e24ba267da65...
*-arm-linux-gnueabihf-debug.tar.gz 9c3063929beb36e8... bc9dc022946e5b7b...
*-arm-linux-gnueabihf.tar.gz b31ec6a01a8f646f... cc7b7c95f52d3857...
*-arm64-apple-darwin-unsigned.dmg 976f869f3a17af74... b6673b7e07af41a3...
*-arm64-apple-darwin-unsigned.tar.gz 9e53f2d2d725869c... ee142515d5288599...
*-arm64-apple-darwin.tar.gz 617e7f24db299fcf... f76a8be090f066ab...
*-powerpc64-linux-gnu-debug.tar.gz ece049f36398a61f... 60b7dc310cbdb278...
*-powerpc64-linux-gnu.tar.gz 4c41c03295d5e1bd... e66b1f029e13246d...
*-powerpc64le-linux-gnu-debug.tar.gz 73a48e1cc36cc074... 2f0cc090e1ae4fda...
*-powerpc64le-linux-gnu.tar.gz ae2e7655bff5f833... 368799c69d54f90b...
*-riscv64-linux-gnu-debug.tar.gz 317cfaf9eb278e16... 7c504186bf78f53f...
*-riscv64-linux-gnu.tar.gz 8e2930c98099f8c8... 919e76089337f778...
*-win64-debug.zip c1b38de5f2d91cd0... 66e037b52a2db085...
*-win64-setup-unsigned.exe 7b02d4e5905a5550... 8c337e4132de70b0...
*-win64-unsigned.tar.gz 09c5874855fa03ab... 8058ba5ae25780db...
*-win64.zip 158479e2e0987a7c... 9cbde8f09ea4e362...
*-x86_64-apple-darwin-unsigned.dmg 01a9abefed526c22... 4389e6a2f5ed6e76...
*-x86_64-apple-darwin-unsigned.tar.gz 89b5c04e3459f654... fb2836ae8977f816...
*-x86_64-apple-darwin.tar.gz 86407596a5a3c987... c8909ae985e2f8a3...
*-x86_64-linux-gnu-debug.tar.gz 1ece469c5ef915d0... df8c44df07e83a72...
*-x86_64-linux-gnu.tar.gz 97d5d292434cae7f... f1529b27556d12ae...
*.tar.gz b4583cf6bbc78e88... ac85ac56cf0e2855...
guix_build.log e45f417c7e72378d... 1ccf618669d93e84...
guix_build.log.diff a4370a200e853dfe...

@sipa
Copy link
Member Author

sipa commented Jan 12, 2023

Anything left to do here?

@maflcko maflcko added this to the 25.0 milestone Jan 12, 2023
@fanquake
Copy link
Member

cc @apoelstra @jonasnick

@Sjors
Copy link
Member

Sjors commented Jan 12, 2023

For other reviewers, including myself, this should be useful for testing a subtree update: https://github.com/bitcoin/bitcoin/blob/master/test/lint/README.md#git-subtree-checksh

@Sjors
Copy link
Member

Sjors commented Jan 12, 2023

ACK 2022917, but 4462cb0 could use more eyes on it.

Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

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

commit message for 4462cb0

misnames secp256k1_context_static as secp256k1_static_context

@@ -14,7 +14,7 @@
</ItemGroup>
<ItemDefinitionGroup>
<ClCompile>
<PreprocessorDefinitions>ENABLE_MODULE_ECDH;ENABLE_MODULE_RECOVERY;ENABLE_MODULE_EXTRAKEYS;ENABLE_MODULE_SCHNORRSIG;%(PreprocessorDefinitions)</PreprocessorDefinitions>
Copy link
Member

Choose a reason for hiding this comment

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

was ecdh enabled for any reason?

Copy link
Member Author

@sipa sipa Jan 12, 2023

Choose a reason for hiding this comment

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

Bitcoin Core doesn't use the libsecp256k1 ecdh module, so it should be disabled. I don't know why it was enabled here before.

@achow101
Copy link
Member

ACK 2022917

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

utACK 2022917

@fanquake fanquake merged commit 07c54de into bitcoin:master Jan 13, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 13, 2023
kwvg added a commit to kwvg/dash that referenced this pull request Sep 8, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
kwvg added a commit to kwvg/dash that referenced this pull request Sep 9, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
kwvg added a commit to kwvg/dash that referenced this pull request Sep 9, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
kwvg added a commit to kwvg/dash that referenced this pull request Sep 24, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
kwvg added a commit to kwvg/dash that referenced this pull request Sep 28, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
UdjinM6 pushed a commit to kwvg/dash that referenced this pull request Nov 15, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
UdjinM6 pushed a commit to kwvg/dash that referenced this pull request Nov 16, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
UdjinM6 pushed a commit to kwvg/dash that referenced this pull request Nov 20, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
PastaPastaPasta pushed a commit to kwvg/dash that referenced this pull request Nov 21, 2023
…on 0.2.0

notes:
- excludes changes made to `SignSchnorr`, `XOnlyPubKey`, kernel context and absent {fuzz,bench} tests
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Nov 21, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants