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

Upstream PRs 1228, 1236, 1243, 1238, 1246, 1247, 1242, 1250, 1244, 1241, 1257, 1226, 1252, 1118, 1245, 1266, 1269 #254

Merged
merged 39 commits into from
Jul 24, 2023
Merged

Upstream PRs 1228, 1236, 1243, 1238, 1246, 1247, 1242, 1250, 1244, 1241, 1257, 1226, 1252, 1118, 1245, 1266, 1269 #254

merged 39 commits into from
Jul 24, 2023

Conversation

jonasnick
Copy link
Contributor

[bitcoin-core/secp256k1#1228]: release cleanup: bump version after 0.3.0
[bitcoin-core/secp256k1#1236]: Update comment for secp256k1_modinv32_inv256
[bitcoin-core/secp256k1#1243]: build: Ensure no optimization when building for coverage analysis
[bitcoin-core/secp256k1#1238]: build: bump CMake minimum requirement to 3.13
[bitcoin-core/secp256k1#1246]: Typo
[bitcoin-core/secp256k1#1247]: Apply Checks only in VERIFY mode.
[bitcoin-core/secp256k1#1242]: Set ARM ASM symbol visibility to hidden
[bitcoin-core/secp256k1#1250]: No need to subtract 1 before doing a right shift
[bitcoin-core/secp256k1#1244]: Suppress -Wunused-parameter when building for coverage analysis
[bitcoin-core/secp256k1#1241]: build: Improve SECP_TRY_APPEND_DEFAULT_CFLAGS macro
[bitcoin-core/secp256k1#1257]: ct: Use volatile "trick" in all fe/scalar cmov implementations
[bitcoin-core/secp256k1#1226]: Add CMake instructions to release process
[bitcoin-core/secp256k1#1252]: Make position of * in pointer declarations in include/ consistent
[bitcoin-core/secp256k1#1118]: Add x-only ecmult_const version with x specified as n/d
[bitcoin-core/secp256k1#1245]: tests: Add Wycheproof ECDSA vectors
[bitcoin-core/secp256k1#1266]: release: Prepare for 0.3.1
[bitcoin-core/secp256k1#1269]: changelog: Fix link

This PR can be recreated with ./contrib/sync-upstream.sh -b sync-upstream range v0.3.1.
Tip: Use git show --remerge-diff to show the changes manually added to the merge commit.

  • Made position of * consistent in -zkp include/ files.

jonasnick and others added 30 commits March 8, 2023 22:07
@jonasnick
release cleanup: bump version after 0.3.0
28e63f7
@real-or-random
Merge bitcoin-core/secp256k1#1228: release cleanup: bump version afte…
5658209 
…r 0.3.0

28e63f7 release cleanup: bump version after 0.3.0 (Jonas Nick)

Pull request description:

  Based on #1223. Should be merged only after tagging the release.

ACKs for top commit:
  sipa:
    ACK 28e63f7
  real-or-random:
    ACK 28e63f7

Tree-SHA512: d219f836c9258af52389f62c167adb79a0f83f520ede514e286e84f0540d35234322e67d582409c332662db17114da1681419d5d400ed88ad2be66a0f6a06089
@roconnor-blockstream
Update comment for secp256k1_modinv32_inv256
647f0a5
@sipa
Merge bitcoin-core/secp256k1#1236: Update comment for secp256k1_modin…
427bc3c 
…v32_inv256

647f0a5 Update comment for secp256k1_modinv32_inv256 (roconnor-blockstream)

Pull request description:

ACKs for top commit:
  sipa:
    ACK 647f0a5
  real-or-random:
    utACK bitcoin-core/secp256k1@647f0a5

Tree-SHA512: 7c2ec02acf985bb6edfc619ce31bd63511ff634d847a25888927b48b5164353a912d470421b0b969a868fbc5b865cbea188e14357557f44be42d5702af7c5a6b
@theuni
build: bump CMake minimum requirement to 3.13
96dd062 
Among other things this allows us to link against object libraries.
@hebasto
build: Ensure no optimization when building for coverage analysis
8e79c7e
@hebasto
Suppress -Wunused-parameter when building for coverage analysis
4429a8c
@hebasto @theuni
Set ARM ASM symbol visibility to hidden
fd2a408 
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
@real-or-random
Merge bitcoin-core/secp256k1#1243: build: Ensure no optimization when…
0cf2fb9 
… building for coverage analysis

8e79c7e build: Ensure no optimization when building for coverage analysis (Hennadii Stepanov)

Pull request description:

  #944 introduced a regression when building for coverage analysis. The `-O2` flag from the default Autoconf's `CFLAGS` overrides the coverage-specific `-O0` one, which makes coverage analysis results [less reliable](https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html).

  This PR restores the pre-#944 behaviour.

  In contrast to an alternative smaller diff:
  ```diff
  --- a/configure.ac
  +++ b/configure.ac
  @@ -240,7 +240,7 @@ fi

   if test x"$enable_coverage" = x"yes"; then
       SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
  -    SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
  +    CFLAGS="$CFLAGS -O0 --coverage "
       LDFLAGS="--coverage $LDFLAGS"
   else
       # Most likely the CFLAGS already contain -O2 because that is autoconf's default.
  ```

  this PR ensures that the user always has the last word.

  FWIW, Bitcoin Core uses a similar [approach](https://github.com/bitcoin/bitcoin/blob/460e394625fab2942748aaeec9be31f460f91c58/configure.ac#L879-L884).

ACKs for top commit:
  jonasnick:
    tested ACK 8e79c7e
  real-or-random:
    utACK 8e79c7e

Tree-SHA512: f04b55921d397bd7c003ec0283101d3908f3fb507789c855e1b6d5abd150e7d6281d5eeb8fefbb7d6a55b3c6f29a19324f570eee009794f8fa9bca956229e7ce
@real-or-random
Merge bitcoin-core/secp256k1#1238: build: bump CMake minimum requirem…
9c8c4f4 
…ent to 3.13

96dd062 build: bump CMake minimum requirement to 3.13 (Cory Fields)

Pull request description:

  As requested here: bitcoin-core/secp256k1#1230 (comment) . Ping @hebasto

  Among other things this allows us to link against object libraries.

  3.13 has been mentioned several times as a good overlap between newish features and widespread Linux availability.

ACKs for top commit:
  hebasto:
    ACK 96dd062
  real-or-random:
    utACK 96dd062

Tree-SHA512: 6c744809aa393b48ef10b3d46c6630370c388a8d375116bfad65c6c907e69c36ed71c1579b9d5c3aa976f70b1cd70e837c1a0226910a43539435125115b32568
@hebasto
Replace SECP256K1_ECMULT_TABLE_VERIFY macro by a function
5bb03c2
@roconnor-blockstream
Typo
d1e7ca1
@roconnor-blockstream
Apply Checks only in VERIFY mode.
4ebd828
@jonasnick
Merge bitcoin-core/secp256k1#1246: Typo
70be3ca 
d1e7ca1 Typo (roconnor-blockstream)

Pull request description:

ACKs for top commit:
  jonasnick:
    ACK d1e7ca1

Tree-SHA512: 0d53ad29cf86921a59aae3953c7d786b7ee0567c9cf92d037853e8c4f7536e45c6b50467eb95d3763f27ae3fd1a8b2b9cf88689f320cb13cebf52f70bb4affef
@sipa
Merge bitcoin-core/secp256k1#1247: Apply Checks only in VERIFY mode.
f16a709 
4ebd828 Apply Checks only in VERIFY mode. (roconnor-blockstream)

Pull request description:

  This is already done in `field_5x52_impl.h`.

ACKs for top commit:
  sipa:
    ACK 4ebd828
  jonasnick:
    ACK 4ebd828

Tree-SHA512: c24211e5219907e41e2c5792255734bd50ca5866a4863abbb3ec174ed92d1792dd10563a94c08e8fecd6cdf776a9c49ca87e8f9806a023d9081ecc0d55ae3e66
@real-or-random
Merge bitcoin-core/secp256k1#1242: Set ARM ASM symbol visibility to `…
464a911 
…hidden`

fd2a408 Set ARM ASM symbol visibility to `hidden` (Hennadii Stepanov)

Pull request description:

  Solves one item in #1181.

  To test on arm-32bit hardware, run:
  ```
  $ ./autogen.sh && ./configure --enable-experimental --with-asm=arm && make
  ```

  On master branch (427bc3c):
  ```
  $ nm -D .libs/libsecp256k1.so | grep secp256k1_fe
  0000e2bc T secp256k1_fe_mul_inner
  0000e8dc T secp256k1_fe_sqr_inner
  ```

  With this PR:
  ```
  $ nm -D .libs/libsecp256k1.so | grep secp256k1_fe | wc -l
  0
  ```

  For reference, see https://sourceware.org/binutils/docs/as/Hidden.html.

ACKs for top commit:
  theuni:
    ACK fd2a408.
  sipa:
    ACK fd2a408

Tree-SHA512: abf8ad332631672c036844f69c5599917c49e12c4402bf9066f93a692d3007b1914bd3eea8f83f0141c1b09d5c88ebc5e6c8bfbb5444b7b3471749f7b901ca59
@real-or-random @hebasto
Add CMake instructions to release process
0c07c82 
... and make wording a bit more consistent.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
@hebasto @real-or-random
build: Improve SECP_TRY_APPEND_DEFAULT_CFLAGS macro
3addb4c 
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
@roconnor-blockstream
No need to subtract 1 before doing a right shift
3e43041
@jonasnick
Merge bitcoin-core/secp256k1#1250: No need to subtract 1 before doing…
1d8f367 
… a right shift

3e43041 No need to subtract 1 before doing a right shift (roconnor-blockstream)

Pull request description:

ACKs for top commit:
  real-or-random:
    utACK 3e43041
  jonasnick:
    ACK 3e43041

Tree-SHA512: bcecda11eae3fb845bef7af88c6171bedcd933872d08a9849c0a250cb6c9e982a88bd45e8a8364a4a348f8be413fc91ee04cf8fa78adae44e584e3ad7ec544cf
@jonasnick
Merge bitcoin-core/secp256k1#1244: Suppress -Wunused-parameter when…
afd8b23 
… building for coverage analysis

5bb03c2 Replace `SECP256K1_ECMULT_TABLE_VERIFY` macro by a function (Hennadii Stepanov)
4429a8c Suppress `-Wunused-parameter` when building for coverage analysis (Hennadii Stepanov)

Pull request description:

ACKs for top commit:
  real-or-random:
    utACK 5bb03c2
  jonasnick:
    ACK 5bb03c2

Tree-SHA512: 19a395434ecefea201a03fc45b3f0b88f1520908926ac1207bbc6570034b1141b49c3c98e66819dcd9069dfdd28c7c6fbe957f13fb6bd178fd57ce65bfbb8fbd
@jonasnick
Merge bitcoin-core/secp256k1#1241: build: Improve `SECP_TRY_APPEND_DE…
2bca0a5 
…FAULT_CFLAGS` macro

3addb4c build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro (Hennadii Stepanov)

Pull request description:

ACKs for top commit:
  real-or-random:
    utACK 3addb4c
  jonasnick:
    ACK 3addb4c

Tree-SHA512: 918d906570d82be9354fba72bb55d50b8f661cf7cd4404dc244deb489c2bca95b3942ae8af830873ba825dc8ddc68b99c973fc984ff13fdd1f6668f412ca56a3
@jonasnick
Make position of * in pointer declarations in include/ consistent
3d1f430
@real-or-random
ct: Use volatile "trick" in all fe/scalar cmov implementations
4a496a3 
Apparently clang 15 is able to compile our cmov code into a branch,
at least for fe_cmov and fe_storage_cmov. This commit makes the
condition volatile in all cmov implementations (except ge but that
one only calls into the fe impls).

This is just a quick fix. We should still look into other methods,
e.g., asm and #457. We should also consider not caring about
constant-time in scalar_low_impl.h

We should also consider testing on very new compilers in nightly CI,
see bitcoin-core/secp256k1#864 (comment)
@real-or-random
Merge bitcoin-core/secp256k1#1257: ct: Use volatile "trick" in all fe…
2d51a45 
…/scalar cmov implementations

4a496a3 ct: Use volatile "trick" in all fe/scalar cmov implementations (Tim Ruffing)

Pull request description:

  Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls).

  This is just a quick fix. We should still look into other methods, e.g., asm and #457. We should also consider not caring about constant-time in scalar_low_impl.h

  We should also consider testing on very new compilers in nightly CI, see bitcoin-core/secp256k1#864 (comment)

ACKs for top commit:
  jonasnick:
    ACK 4a496a3

Tree-SHA512: a6010f9d752e45f01f88b804a9b27e77caf5ddf133ddcbc4235b94698bda41c9276bf588c93710e538250d1a96844bcec198ec5459e675f166ceaaa42da921d5
@jonasnick
Merge bitcoin-core/secp256k1#1226: Add CMake instructions to release …
4e68262 
…process

0c07c82 Add CMake instructions to release process (Tim Ruffing)

Pull request description:

ACKs for top commit:
  hebasto:
    ACK 0c07c82
  jonasnick:
    ACK 0c07c82

Tree-SHA512: a2c38f71cc96766f833f6ed79af1b560501f2d9516843b789de06c9cbffd7a1d9e8709a2f4d08bea8c1c3616301e51942cfa9f11e25e903ee4146c7733a8cb8c
@real-or-random
Merge bitcoin-core/secp256k1#1252: Make position of * in pointer decl…
a0f4644 
…arations in include/ consistent

3d1f430 Make position of * in pointer declarations in include/ consistent (Jonas Nick)

Pull request description:

ACKs for top commit:
  sipa:
    utACK 3d1f430. I have not verified these are the only instances where changes would need to be made.
  apoelstra:
    utACK 3d1f430 from me too. I also value consistency more than either specific choice.'
  real-or-random:
    utACK bitcoin-core/secp256k1@3d1f430

Tree-SHA512: 6361880f4a47e58c83623f094dd121882752fa805e275033cd638d1e8d3477ade9037e5d9e34a57ae46013848648bd7ab764cad326133f2d3435c9a70a0c841b
@sipa
Add x-only ecmult_const version for x=n/d
4485926
@sipa
Add exhaustive tests for ecmult_const_xonly
0f86420
@RandomLattice @andozw
tests: Add Wycheproof ECDSA vectors
e5de454 
Adds a test using the Wycheproof vectors as outlined in #1106. The
vectors are taken from the Wycheproof repo. We use a python script
to convert the JSON-formatted vectors into C code.

Co-authored-by: Sean Andersen <6730974+andozw@users.noreply.github.com>
real-or-random and others added 8 commits April 10, 2023 08:24
@real-or-random
Merge bitcoin-core/secp256k1#1118: Add x-only ecmult_const version wi…
145078c 
…th x specified as n/d

0f86420 Add exhaustive tests for ecmult_const_xonly (Pieter Wuille)
4485926 Add x-only ecmult_const version for x=n/d (Pieter Wuille)

Pull request description:

  This implements a generalization of Peter Dettman's sqrt-less x-only random-base multiplication algorithm from BlockstreamResearch#262, using the Jacobi symbol algorithm from #979. The generalization is to permit the X coordinate of the base point to be specified as a fraction $n/d$:

  To compute $x(q \cdot P)$, where $x(P) = n/d$:
  * Compute $g=n^3 + 7d^3$.
  * Let $P' = (ng, g^2, 1)$ (the Jacobian coordinates of $P$ mapped to the isomorphic curve $y^2 = x^3 + 7(dg)^3$).
  * Compute the Jacobian coordinates $(X',Y',Z') = q \cdot P'$ on the isomorphic curve.
  * Return $X'/(dgZ'^2)$, which is the affine x coordinate on the isomorphic curve $X/Z'^2$ mapped back to secp256k1.

  This ability to specify the X coordinate as a fraction is useful in the context of x-only [Elligator Swift](https://eprint.iacr.org/2022/759), which can decode to X coordinates on the curve without inversions this way.

ACKs for top commit:
  jonasnick:
    ACK 0f86420
  real-or-random:
    ACK 0f86420

Tree-SHA512: eeedb3045bfabcb4bcaf3a1738067c83a5ea9a79b150b8fd1c00dc3f68505d34c19654885a90e2292ae40ddf40a58dfb27197d98eebcf5d6d9e25897e07ae595
@sipa
Merge bitcoin-core/secp256k1#1245: tests: Add Wycheproof ECDSA vectors
7b7503d 
e5de454 tests: Add Wycheproof ECDSA vectors (RandomLattice)

Pull request description:

  This PR adds a test using the Wycheproof vectors as outlined in #1106. We add all 463 ECDSA test vectors. These vectors cover:
  - edge cases in arithmetic operations
  - signatures with special values for (r,s) that should be rejected
  - special cases of public keys

  The vectors are pulled from the Wycheproof project using a python script to emit C code.

  All the new ECDSA Wycheproof vectors pass.

ACKs for top commit:
  sipa:
    ACK e5de454
  real-or-random:
    ACK e5de454

Tree-SHA512: e9684f14ff3f5225a4a4949b490e07527d559c28aa61ed03c03bc52ea64785f0b80b9e1b1628665eacf24006526271ea0fb108629c9c3c1d758e52d214a056f1
@real-or-random @sipa
changelog: Catch up in preparation of 0.3.1
0e09166 
Co-authored-by: Pieter Wuille <pieter@wuille.net>
@real-or-random
changelog: Remove inconsistent newlines
1d9a13f
@real-or-random
release: Prepare for 0.3.1
898e1c6
@jonasnick
Merge bitcoin-core/secp256k1#1266: release: Prepare for 0.3.1
ec98fce 
898e1c6 release: Prepare for 0.3.1 (Tim Ruffing)
1d9a13f changelog: Remove inconsistent newlines (Tim Ruffing)
0e09166 changelog: Catch up in preparation of 0.3.1 (Tim Ruffing)

Pull request description:

ACKs for top commit:
  sipa:
    ACK 898e1c6
  jonasnick:
    ACK 898e1c6

Tree-SHA512: 941b1524f8b34ca803a2ede55a7baf54d2faa69a4c5e47254297e96cc4ac2121094ed90e7cd64a708f3e9af830b0de0ef3c755dfee1b01ce958cc998fc1a1311
@real-or-random
changelog: Fix link
6a37b2a
@jonasnick
Merge bitcoin-core/secp256k1#1269: changelog: Fix link
346a053 
6a37b2a changelog: Fix link (Tim Ruffing)

Pull request description:

Top commit has no ACKs.

Tree-SHA512: 70d50c8fe958a197eb527e51c6f8120609e3166d93bfc1bbec75a3cb565c406d5ba0e6d088a724dcfda422b6594abf53f507211946a0533515df371d5d2a91bf
@real-or-random
Copy link
Collaborator

Made position of * consistent in -zkp include/ files.

git grep "[a-zA-Z]\* " include shows more locations that need to be modified.

@real-or-random
Copy link
Collaborator

ACK otherwise

@jonasnick
Copy link
Contributor Author

Oops sorry, should be fixed now.

❯ git grep "[a-zA-Z]\* " include
include/secp256k1.h: *  This context object offers *only limited functionality* , i.e., it cannot be used
include/secp256k1_surjectionproof.h: *  stack; please *do not* use these internals directly.
include/secp256k1_surjectionproof.h: * This data type is *not* opaque. It will always be 32 bytes of whatever
include/secp256k1_whitelist.h: *  stack; please *do not* use these internals directly. To learn the number

real-or-random
real-or-random approved these changes Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@real-or-random real-or-random left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK a9a5c24

still testing

real-or-random
real-or-random approved these changes Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@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.

ACK a9a5c24

@real-or-random real-or-random merged commit a9a5c24 into BlockstreamResearch:sync-upstream Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@real-or-random real-or-random real-or-random approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@jonasnick @real-or-random @roconnor-blockstream @sipa @theuni @hebasto @RandomLattice