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

Update libsecp256k1 subtree #4332

Merged
merged 8 commits into from
Aug 11, 2021
Merged

Update libsecp256k1 subtree #4332

merged 8 commits into from
Aug 11, 2021

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 11, 2021

Few notes:

  • 14047 and 12461 are needed for updates to be merged cleanly
  • 22448 is a partial one because there are schnorr key/pubkey related changes in the original PR

laanwj and others added 8 commits August 10, 2021 23:55
@laanwj @UdjinM6
Merge bitcoin#14047: Add HKDF_HMAC256_L32 and method to negate a priv…
c4f7bb5 
…ate key

8794a4b QA: add test for HKDF HMAC_SHA256 L32 (Jonas Schnelli)
551d489 Add HKDF HMAC_SHA256 L=32 implementations (Jonas Schnelli)
3b64f85 QA: add test for CKey::Negate() (Jonas Schnelli)
463921b CKey: add method to negate the key (Jonas Schnelli)

Pull request description:

  This adds a limited implementation of `HKDF` (defined by rfc5869) that supports only HMAC-SHA256  and length output of 32 bytes (will be required for v2 transport protocol).

  This PR also includes a method to negate a private key which is useful to enforce public keys starting with 0x02 (or 0x03) (a requirement for the v2 transport protocol). The new `CKey::Negate()` method is pretty much a wrapper around `secp256k1_ec_privkey_negate()`.

  Including tests.

  This is a subset of bitcoin#14032 and a pre-requirement for the v2 transport protocol.

ACKs for commit 8794a4:

Tree-SHA512: 5341929dfa29f5da766ec3612784baec6a3ad69972f08b5a985a8aafdae4dae36f104a2b888d1f5d1f33561456bd111f960d7e32c2cc4fd18e48358468f26c1a
@laanwj @UdjinM6
Merge bitcoin#12461: scripted-diff: Rename key size consts to be rela…
81b8eb4 
…tive to their class

0580f86 Fixup whitespace (Ben Woosley)
47101bb scripted-diff: Rename CPubKey and CKey::*_KEY_SIZE and COMPRESSED_*_KEY_SIZE (Ben Woosley)

Pull request description:

  ~~And introduce CPubKeySig to host code relative to key sigs.~~

ACKs for top commit:
  meshcollider:
    utACK bitcoin@0580f86

Tree-SHA512: 29aa0be54912358b138e391b9db78639786f56580493e590ec9f773c0e1b421740133d05a79be247c7ee57e71c9c9e41b9cb54088cb3c0e3f813f74f0895287b
@fanquake @UdjinM6
Merge bitcoin#19228: Update libsecp256k1 subtree
9d36ba6 
e10439c scripted-diff: rename privkey with seckey in secp256k1 interface (Pieter Wuille)
ca8bc42 Drop --disable-jni from libsecp256k1 configure options (Pieter Wuille)
ddc2419 Update MSVC build config for libsecp256k1 (Pieter Wuille)
67f232b Squashed 'src/secp256k1/' changes from b19c000..2ed54da (Pieter Wuille)

Pull request description:

  It's been abound a year since the subtree was updated.

  Here is a list of the included PRs:

  * bitcoin-core/secp256k1#755: Recovery signing: add to constant time test, and eliminate non ct operators
  * bitcoin-core/secp256k1#754: Fix uninit values passed into cmov
  * bitcoin-core/secp256k1#752: autoconf: Use ":" instead of "dnl" as a noop
  * bitcoin-core/secp256k1#750: Add macOS to the CI
  * bitcoin-core/secp256k1#701: Make ec_ arithmetic more consistent and add documentation
  * bitcoin-core/secp256k1#732: Retry if r is zero during signing
  * bitcoin-core/secp256k1#742: Fix typo in ecmult_const_impl.h
  * bitcoin-core/secp256k1#740: Make recovery/main_impl.h non-executable
  * bitcoin-core/secp256k1#735: build: fix OpenSSL EC detection on macOS
  * bitcoin-core/secp256k1#728: Suppress a harmless variable-time optimization by clang in memczero
  * bitcoin-core/secp256k1#722: Context isn't freed in the ECDH benchmark
  * bitcoin-core/secp256k1#700: Allow overriding default flags
  * bitcoin-core/secp256k1#708: Constant-time behaviour test using valgrind memtest.
  * bitcoin-core/secp256k1#710: Eliminate harmless non-constant time operations on secret data.
  * bitcoin-core/secp256k1#718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1
  * bitcoin-core/secp256k1#714: doc: document the length requirements of output parameter.
  * bitcoin-core/secp256k1#682: Remove Java Native Interface
  * bitcoin-core/secp256k1#713: Docstrings
  * bitcoin-core/secp256k1#704: README: add a section for test coverage
  * bitcoin-core/secp256k1#709: Remove secret-dependant non-constant time operation in ecmult_const.
  * bitcoin-core/secp256k1#703: Overhaul README.md
  * bitcoin-core/secp256k1#689: Remove "except in benchmarks" exception for fp math
  * bitcoin-core/secp256k1#679: Add SECURITY.md
  * bitcoin-core/secp256k1#685: Fix issue where travis does not show the ./tests seed…
  * bitcoin-core/secp256k1#690: Add valgrind check to travis
  * bitcoin-core/secp256k1#678: Preventing compiler optimizations in benchmarks without a memory fence
  * bitcoin-core/secp256k1#688: Fix ASM setting in travis
  * bitcoin-core/secp256k1#684: Make no-float policy explicit
  * bitcoin-core/secp256k1#677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var
  * bitcoin-core/secp256k1#647: Increase robustness against UB in secp256k1_scalar_cadd_bit
  * bitcoin-core/secp256k1#664: Remove mention of ec_privkey_export because it doesn't exist
  * bitcoin-core/secp256k1#337: variable sized precomputed table for signing
  * bitcoin-core/secp256k1#661: Make ./configure string consistent
  * bitcoin-core/secp256k1#657: Fix a nit in the recovery tests
  * bitcoin-core/secp256k1#650: secp256k1/src/tests.c:  Properly handle sscanf return value
  * bitcoin-core/secp256k1#654: Fix typo (∞)
  * bitcoin-core/secp256k1#583: JNI: fix use sig array
  * bitcoin-core/secp256k1#644: Avoid optimizing out a verify_check
  * bitcoin-core/secp256k1#652: README.md: update instruction to run tests
  * bitcoin-core/secp256k1#651: Fix typo in secp256k1_preallocated.h
  * bitcoin-core/secp256k1#640: scalar_impl.h: fix includes
  * bitcoin-core/secp256k1#655: jni: Use only Guava for hex encoding and decoding
  * bitcoin-core/secp256k1#634: Add a descriptive comment for secp256k1_ecmult_const.
  * bitcoin-core/secp256k1#631: typo in comment for secp256k1_ec_pubkey_tweak_mul ()
  * bitcoin-core/secp256k1#629: Avoid calling _is_zero when _set_b32 fails.
  * bitcoin-core/secp256k1#630: Note intention of timing sidechannel freeness.
  * bitcoin-core/secp256k1#628: Fix ability to compile tests without -DVERIFY.
  * bitcoin-core/secp256k1#627: Guard memcmp in tests against mixed size inputs.
  * bitcoin-core/secp256k1#578: Avoid implementation-defined and undefined behavior when dealing with sizes
  * bitcoin-core/secp256k1#595: Allow to use external default callbacks
  * bitcoin-core/secp256k1#600: scratch space: use single allocation
  * bitcoin-core/secp256k1#592: Use trivial algorithm in ecmult_multi if scratch space is small
  * bitcoin-core/secp256k1#566: Enable context creation in preallocated memory
  * bitcoin-core/secp256k1#596: Make WINDOW_G configurable
  * bitcoin-core/secp256k1#561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config
  * bitcoin-core/secp256k1#533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...)
  * bitcoin-core/secp256k1#617: Pass scalar by reference in secp256k1_wnaf_const()
  * bitcoin-core/secp256k1#619: Clear a copied secret key after negation
  * bitcoin-core/secp256k1#612: Allow field_10x26_arm.s to compile for ARMv7 architecture

ACKs for top commit:
  real-or-random:
    ACK e10439c I verified the diff (subtree matches my local tree, manual inspection of other commits) but I didn't tested the resulting code
  fanquake:
    ACK e10439c
  Sjors:
    ACK e10439c
  jonasnick:
    reACK e10439c

Tree-SHA512: eb6284a485da78e9d2ed3f771df85560d47c770ebf480a0d4121ab356ad26be101a2b973efe412f26e6c142bc1dbd2efbb5cc08774233e41918c59fe3dff3387
@fanquake @UdjinM6
Merge bitcoin#19944: Update secp256k1 subtree (including BIP340 support)
c2ab828 
b9c1a76 Squashed 'src/secp256k1/' changes from 2ed54da..8ab24e8 (Pieter Wuille)

Pull request description:

  This updates our src/secp256k1 subtree to the latest libsecp256k1 upstream version.

  As it adds BIP340 support (see bitcoin-core/secp256k1#558), this is a prerequisite for bitcoin#17977. In particular, it contains:
  * A few generic library improvements
  * Support for x-only public keys as used by BIP340.
  * Support for "key pair" objects, making signing more efficient by using a precomputed public key.
  * Signing support for BIP340 Schnorr (single-party) signatures.
  * Verification support for BIP340 Schnorr signatures.
  * Support for verifying tweaked x-only keys, as used by BIP341's Taproot construction.

  Things that are not included:
  * MuSig, nor any kind of multisignatures, threshold signatures, ... on top.
  * Batch verification.
  * Support for variable-length messages in BIP340 (which are still being discussed, but won't affect BIP341, or Bitcoin Core).
  * A few more generic improvements that are still in the pipeline, including faster modular inversions.

ACKs for top commit:
  instagibbs:
    ACK 894fb33
  fanquake:
    ACK 894fb33. Any Valgrind concerns will be addressed upstream, see discussion in bitcoin-core/secp256k1#813, and if necessary, can be pulled into our tree prior to the 0.21.0 branch off. They are not a blocker for merging this PR in it's current state.
  benthecarman:
    ACK `894fb33`

Tree-SHA512: 6dc992f4477069b7fbd223316f1be955750923be1479c38adad2312649fdca1f316edb375c42ef9d97cea2407caaef49fb8c93abd6c037fe1a522910cbbc2479
@fanquake @UdjinM6
Merge bitcoin#20147: Update libsecp256k1 (endomorphism, test improvem…
63ee049 
…ents)

52380bf Squashed 'src/secp256k1/' changes from 8ab24e8..c6b6b8f (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
@fanquake @UdjinM6
Merge bitcoin#20257: Update secp256k1 subtree to latest master
ee2a08f 
6c0259f Squashed 'src/secp256k1/' changes from c6b6b8f..3967d96 (Pieter Wuille)

Pull request description:

  Nothing important changed, but this silences this (erroneous) warning in certain GCC 9 versions:

  ```
  In file included from src/secp256k1.c:16:
  src/ecmult_impl.h: In function ‘secp256k1_ecmult’:
  src/ecmult_impl.h:496:48: warning: array subscript [1, 268435456] is outside array bounds of ‘struct secp256k1_strauss_point_state[1]’ [-Warray-bounds]
    496 |             secp256k1_gej tmp = a[state->ps[np].input_pos];
        |                                   ~~~~~~~~~~~~~^~~~~~~~~~
  src/ecmult_impl.h:565:42: note: while referencing ‘ps’
    565 |     struct secp256k1_strauss_point_state ps[1];
        |                                          ^~
  src/ecmult_impl.h:502:139: warning: array subscript [1, 268435456] is outside array bounds of ‘struct secp256k1_strauss_point_state[1]’ [-Warray-bounds]
    502 |             secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
        |                                                                                                                              ~~~~~~~~~~~~~^~~~~~~~~~
  src/ecmult_impl.h:565:42: note: while referencing ‘ps’
    565 |     struct secp256k1_strauss_point_state ps[1];
        |                                          ^~
  ```

  (see bitcoin-core/secp256k1#834)

ACKs for top commit:
  fanquake:
    ACK 5803f5f  - performed the update myself and got the same change: [check_20257_subtree](https://github.com/fanquake/bitcoin/tree/check_20257_subtree).
  hebasto:
    ACK 5803f5f, tested on Linux Mint 20 (x86_64) with `gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0` -- no warnings are emitted.

Tree-SHA512: 386281d23aee93a3b1d1a09fec8319c3a477e46967430c935677eed54abddc62d5a7710f9eeab1ec476ace05adcb194b5b377712e44a6bb95a74ffa35faf77f3
@laanwj @UdjinM6
Merge bitcoin#21573: Update libsecp256k1 subtree to latest master
bc61867 
5c7ee1b libsecp256k1 no longer has --with-bignum= configure option (Pieter Wuille)
bdca9bc Squashed 'src/secp256k1/' changes from 3967d96..efad350 (Pieter Wuille)
cabb566 Disable certain false positive warnings for libsecp256k1 msvc build (Pieter Wuille)

Pull request description:

  This updates our src/secp256k1 subtree to the latest upstream master. The changes include:

  * The introduction of safegcd-based modular inverses, reducing ECDSA signing time by 25%-30% and ECDSA verification time by 15%-17%.
    * [Original paper](https://gcd.cr.yp.to/papers.html) by Daniel J. Bernstein and Bo-Yin Yang
    * [Implementation](bitcoin-core/secp256k1#767) by Peter Dettman; [final](bitcoin-core/secp256k1#831) version
    * [Explanation](https://github.com/bitcoin-core/secp256k1/blob/master/doc/safegcd_implementation.md) of the algorithm using Python snippets
    * [Analysis](https://github.com/sipa/safegcd-bounds) of the maximum number of iterations the algorithm needs
    * [Formal proof in Coq](https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348) by Russell O'Connor, for a high-level equivalent algorithm
  * Removal of libgmp as an (optional) dependency (which wasn't used in the Bitcoin Core build)
  * CI changes (Travis -> Cirrus)
  * Build system improvements

ACKs for top commit:
  laanwj:
    Tested ACK 5c7ee1b

Tree-SHA512: ad8ac3746264d279556a4aa7efdde3733e114fdba8856dd53218588521f04d83950366f5c1ea8fd56329b4c7fe08eedf8e206f8f26dbe3f0f81852e138655431
@fanquake @UdjinM6
Partial merge bitcoin#22448: Update libsecp256k1 subtree to latest up…
54f3b66 
…stream

c020cba Squashed 'src/secp256k1/' changes from efad350..be8d9c2 (Pieter Wuille)

Pull request description:

  This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes:
  * New schnorrsig API (bitcoin-core/secp256k1#844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted.
  * Don't use asm optimizations for `gen_context` (bitcoin-core/secp256k1#965). This fixes bitcoin#22441.
  * Various testing/CI improvements

ACKs for top commit:
  hebasto:
    ACK e4ffb44
  jonatack:
    Light ACK e4ffb44 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes
  fanquake:
    ACK e4ffb44

Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
@UdjinM6 UdjinM6 added this to the 18 milestone Aug 11, 2021
PastaPastaPasta
PastaPastaPasta approved these changes Aug 11, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit 2c1f6ec into dashpay:develop Aug 11, 2021
@kwvg kwvg mentioned this pull request Aug 26, 2023
Merged
@kwvg kwvg mentioned this pull request Sep 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@UdjinM6 @PastaPastaPasta @laanwj @fanquake