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

Upstream PRs 1426, 1430, 1184, 1437, 1442, 1441, 1445, 1438, 1393, 1446, 1450, 1451, 1431, 990, 1455, 1380, 1465, 1466, 1473, 1474, 1476, 1480, 1468, 1482, 1249 #285

Merged
merged 86 commits into from
Jan 23, 2024

Conversation

real-or-random
Copy link
Collaborator

[bitcoin-core/secp256k1#1426]: ci/cirrus: Add native ARM64 jobs
[bitcoin-core/secp256k1#1430]: README: remove CI badge
[bitcoin-core/secp256k1#1184]: Signed-digit based ecmult_const algorithm
[bitcoin-core/secp256k1#1437]: ci: Ignore internal errors of snapshot compilers
[bitcoin-core/secp256k1#1442]: Return temporaries to being unsigned in secp256k1_fe_sqr_inner
[bitcoin-core/secp256k1#1441]: asm: add .note.GNU-stack section for non-exec stack
[bitcoin-core/secp256k1#1445]: bench: add --help option to bench_internal
[bitcoin-core/secp256k1#1438]: correct assertion for secp256k1_fe_mul_inner
[bitcoin-core/secp256k1#1393]: Implement new policy for VERIFY_CHECK and #ifdef VERIFY (issue #1381)
[bitcoin-core/secp256k1#1446]: field: Remove x86_64 asm
[bitcoin-core/secp256k1#1450]: Add group.h ge/gej equality functions
[bitcoin-core/secp256k1#1451]: changelog: add entry for "field: Remove x86_64 asm"
[bitcoin-core/secp256k1#1431]: Add CONTRIBUTING.md
[bitcoin-core/secp256k1#990]: Add comment on length checks when parsing ECDSA sigs
[bitcoin-core/secp256k1#1455]: doc: improve secp256k1_fe_set_b32_mod doc
[bitcoin-core/secp256k1#1380]: Add ABI checking tool for release process
[bitcoin-core/secp256k1#1465]: release: prepare for 0.4.1
[bitcoin-core/secp256k1#1466]: release cleanup: bump version after 0.4.1
[bitcoin-core/secp256k1#1473]: Fix typos
[bitcoin-core/secp256k1#1474]: tests: restore scalar_mul test
[bitcoin-core/secp256k1#1476]: include: make docs more consistent
[bitcoin-core/secp256k1#1480]: Get rid of untested sizeof(secp256k1_ge_storage) == 64 code path
[bitcoin-core/secp256k1#1468]: v0.4.1 release aftermath
[bitcoin-core/secp256k1#1482]: build: Clean up handling of module dependencies
[bitcoin-core/secp256k1#1249]: cmake: Add SECP256K1_LATE_CFLAGS configure option

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


This also cherry-picks a few commits from https://github.com/jonasnick/secp256k1-zkp/commits/musig2-upstream/ and one bitcoin-core/secp256k1#1479 .

real-or-random and others added 30 commits October 17, 2021 12:02
I claim the check can be removed but I don't want to touch this
stable and well-tested code.

On the way, we fix grammar in another comment.
fa4d6c7 ci/cirrus: Add native ARM64 persistent workers (MarcoFalke)
2262d0e ci/cirrus: Bring back skeleton .cirrus.yml without jobs (Tim Ruffing)

Pull request description:

ACKs for top commit:
  real-or-random:
    ACK fa4d6c7
  hebasto:
    re-ACK fa4d6c7, only last two commits have been squashed since my recent [review](bitcoin-core/secp256k1#1426 (review)).

Tree-SHA512: d1fee99d54a41a4126f7eb72695a56137c925dc9ce7cd692a60ea1262ac0789bbd6aa4e4dfc030f0d97d06aeeae0724a5f2d794a85ff533c6cf3cd215f6a4b7a
We're not solely using cirrus anymore and github already displays the CI status
at a different location.
5dab0ba README: remove CI badge (Jonas Nick)

Pull request description:

ACKs for top commit:
  sipa:
    utACK 5dab0ba
  real-or-random:
    utACK 5dab0ba

Tree-SHA512: 56730fa8067cc48b8e5af6fc21b0cd6c47f615c5ebba9edcf29ca5eaf7b2359662a9af219612e80688d8f8939649c7c3c26136c0442ba47d56251a0d92cf984a
* add test case for a=infinity

  The corresponding ecmult_const branch was not tested before this commit.

* add test for edge cases
Based on the surrounding asserts, 112 bits before this line, and 61 bits after this line, this assertion should be 113 bits.  Notably the commensurate line in secp256k1_fe_sqr_inner is correctly assert to be 113 bits.
…gorithm

355bbdf Add changelog entry for signed-digit ecmult_const algorithm (Pieter Wuille)
21f49d9 Remove unused secp256k1_scalar_shr_int (Pieter Wuille)
115fdc7 Remove unused secp256k1_wnaf_const (Pieter Wuille)
aa9f3a3 ecmult_const: add/improve tests (Jonas Nick)
4d16e90 Signed-digit based ecmult_const algorithm (Pieter Wuille)
ba523be make SECP256K1_SCALAR_CONST reduce modulo exhaustive group order (Pieter Wuille)
2140da9 Add secp256k1_scalar_half for halving scalars (+ tests/benchmarks). (Pieter Wuille)

Pull request description:

  Using some insights learned from #1058, this replaces the fixed-wnaf ecmult_const algorithm with a signed-digit based one. Conceptually both algorithms are very similar, in that they boil down to summing precomputed odd multiples of the input points. Practically however, the new algorithm is simpler because it's just using scalar operations, rather than relying on wnaf machinery with skew terms to guarantee odd multipliers.

  The idea is that we can compute $q \cdot A$ as follows:
  * Let $s = f(q)$, for some function $f()$.
  * Compute $(s_1, s_2)$ such that $s = s_1 + \lambda s_2$, using `secp256k1_scalar_lambda_split`.
  * Let $v_1 = s_1 + 2^{128}$ and $v_2 = s_2 + 2^{128}$ (such that the $v_i$ are positive and $n$ bits long).
  * Computing the result as $$\sum_{i=0}^{n-1} (2v_1[i]-1) 2^i A + \sum_{i=0}^{n-1} (2v_2[i]-1) 2^i \lambda A$$ where $x[i]$ stands for the *i*'th bit of $x$, so summing positive and negative powers of two times $A$, based on the bits of $v_1.$

  The comments in `ecmult_const_impl.h` show that if $f(q) = (q + (1+\lambda)(2^n - 2^{129} - 1))/2 \mod n$, the result will equal $q \cdot A$.

  This last step can be performed in groups of multiple bits at once, by looking up entries in a precomputed table of odd multiples of $A$ and $\lambda A$, and then multiplying by a power of two before proceeding to the next group.

  The result is slightly faster (I measure ~2% speedup), but significantly simpler as it only uses scalar arithmetic to determine the table lookup values. The speedup is due to the fact that no skew corrections at the end are needed, and less overhead to determine table indices. The precomputed table sizes are also made independent from the `ecmult` ones, after observing that the optimal table size is bigger here (which also gives a small speedup).

ACKs for top commit:
  jonasnick:
    ACK 355bbdf
  siv2r:
    ACK 355bbdf
  real-or-random:
    ACK 355bbdf

Tree-SHA512: 13db572cb7f9be00bf0931c65fcd8bc8b5545be86a8c8700bd6a79ad9e4d9e5e79e7f763f92ca6a91d9717a355f8162204b0ea821b6ae99d58cb400497ddc656
…shot compilers

8185e72 ci: Ignore internal errors in snapshot compilers (Hennadii Stepanov)

Pull request description:

  It was discussed on today's IRC meeting.

ACKs for top commit:
  real-or-random:
    ACK 8185e72

Tree-SHA512: 0f41ca8303bd3d6efefcd3a544c7bd7dfcf464c57c779c876da4a77cacd262e6c963449d493fdf5a641b0d10b655c8c67fe8a147145b6533328d7bf5344313e1
With this in place, we no-longer see warnings like the following:
```bash
/usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: warning: field_10x26_arm.o: missing .note.GNU-stack section implies executable stack
/usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
```

Should close #1434.
These temporaries seem to been inadvertently changed to signed during a refactoring.  Generally, bit shifting is frowned upon for signed values.
These changes bring the checks to the same values used at the corresponding positions in secp256k1_fe_sqr_inner.
…ed in secp256k1_fe_sqr_inner

1027135 Return temporaries to being unsigned in secp256k1_fe_sqr_inner (roconnor-blockstream)

Pull request description:

  These temporaries seem to been inadvertently changed to signed during a refactoring.  Generally, bit shifting is frowned upon for signed values.

ACKs for top commit:
  sipa:
    utACK 1027135
  real-or-random:
    utACK 1027135

Tree-SHA512: a9fefe4b146163209662cd435422beb3c9561eb9e83110454184f70df2292992f39ec1971143428e039a80cad2f6285db74de2f059e877ad8756ff739269b67a
…or non-exec stack

33dc7e4 asm: add .note.GNU-stack section for non-exec stack (fanquake)

Pull request description:

  With this in place, we no-longer see warnings like the following:
  ```bash
  /usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: warning: field_10x26_arm.o: missing .note.GNU-stack section implies executable stack
  /usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
  ```

  Should close #1434.

ACKs for top commit:
  sipa:
    utACK 33dc7e4
  real-or-random:
    utACK 33dc7e4

Tree-SHA512: f75ded8d971f54d1e871bcc4d815ba367b3e154eea2f18309ecaf9053e22f986bfffcf28418367f8055b65a5a0b245fee045adfcb63a2196df5e2f3aa6c97b89
Widely available versions of GCC and Clang beat our field asm on -O2.
In particular, GCC 10.5.0, which is Bitcoin Core's current compiler
for official x86_64 builds, produces code that is > 20% faster for
fe_mul and > 10% faster for signature verification (see #726).

These are the alternatives to this PR:

We could replace our current asm with the fastest compiler output
that we can find. This is potentially faster, but it has multiple
drawbacks:
 - It's more coding work because it needs detailed benchmarks (e.g.,
   with many compiler/options).
 - It's more review work because we need to deal with inline asm
   (including clobbers etc.) and there's a lack of experts reviewers
   in this area.
 - It's not unlikely that we'll fall behind again in a few compiler
   versions, and then we have to deal with this again, i.e., redo the
   benchmarks. Given our history here, I doubt that we'll revolve
   this timely.

We could change the default of the asm build option to off. But this
will also disable the scalar asm, which is still faster.

We could split the build option into two separate options for field
and scalar asm and only disable the field asm by default. But this
adds complexity to the build and to the test matrix.

My conclusion is that this PR gets the low-hanging fruit in terms of
performance. It simplifies our code significantly. It's clearly an
improvement, and it's very easy to review. Whether re-introducing
better asm (whether from a compiler or from CryptOpt) is worth the
hassle can be evaluated separately, and should not hold up this
improvement.

Solves #726.
because we don't know whether it's an optimization.
…internal

1ddd76a bench: add --help option to bench_internal (Sebastian Falbesoner)

Pull request description:

  While coming up with commands for running the benchmarks for issue bitcoin-core/secp256k1#726 (comment), I noticed that in contrast to `bench{_ecmult}`, `bench_internal` doesn't have a help option yet and figured it would be nice to have one. A comparable past PR is bitcoin-core/secp256k1#1008. Benchmark categories appear in the same order as they are executed, the concrete benchmark names in parantheses per category are listed in alphabetical order.

ACKs for top commit:
  real-or-random:
    utACK 1ddd76a
  siv2r:
    ACK 1ddd76a, tested the `--help` option locally, and it works as expected.

Tree-SHA512: d117641a5f25a7cbf83881f3acceae99624528a0cbb2405efdbe1a3a2762b4d6b251392e954aaa32f6771069d31143743770fccafe198084c12258dedb0856fc
…_mul_inner

dcdda31 Tighten secp256k1_fe_mul_inner's VERIFY_BITS checks (Russell O'Connor)
8e2a5fe correct assertion for secp256k1_fe_mul_inner (roconnor-blockstream)

Pull request description:

  Based on the surrounding asserts, 112 bits before this line, and 61 bits after this line, this assertion should be 113 bits.  Notably the commensurate line in secp256k1_fe_sqr_inner is correctly assert to be 113 bits.

ACKs for top commit:
  real-or-random:
    ACK dcdda31 tested with asm disabled

Tree-SHA512: c35170e37d9a6d1413dd625032028129ab2eccee7da86697ab9641b68ad78efd7251953d51e7acaefd14888d3fd61877f9f05349c44f6fc0133ce9b3921b0e1a
As suggested in issue #1381, this will make things simpler and
improve code readability, as we don't need to force omitting of
evaluations on a case-by-case basis anymore and hence can remove
lots of `#ifdef VERIFY`/`#endif` lines (see next commit). Plus,
VERIFY_CHECK behaves now identical in both non-VERIFY and coverage mode,
making the latter not special anymore and hopefully decreasing
maintenance burden. The idea of "side-effect safety" is given up.

Note that at two places in the ellswift module void-casts of return
values have to be inserted for non-VERIFY builds, in order to avoid
   "variable ... set but not used [-Wunused-but-set-variable]"
warnings.
Now that the `VERIFY_CHECK` compiles to empty in non-VERIFY mode, blocks
that only consist of these macros don't need surrounding `#ifdef VERIFY`
conditions anymore.

At some places intentional blank lines are inserted for grouping and
better readadbility.
jonasnick and others added 15 commits January 4, 2024 17:15
Replaces "ctx: a secp256k1 context object" with "ctx: pointer to a context
object". Also removes the word "existing".
This gets rid of an untested code path. Resolves #1352.

secp256k1_ge_storage is a struct with two secp256k1_fe_storage fields.
The C standard allows the compiler to add padding between the fields and
at the end of the struct, but no sane compiler in the end would do this:
The only reason to add padding is to ensure alignment, but such padding
is never necessary between two fields of the same type.

Similarly, secp256k1_fe_storage is a struct with a single array of
uintXX_t. No padding is allowed between array elements. Again, C allows
the compiler to insert padding at the end of the struct, but there's no
absolute reason to do so in this case.

For the uintXX_t itself, this guaranteed to have no padding bits, i.e.,
it's guaranteed to have exactly XX bits.

So I claim that for any existing compiler in the real world,
sizeof(secp256k1_ge_storage) == 64.
This also splits the big "&&" expression into separate expressions. If
we ever see an assertion fail, the error message will tell it precisely
which one failed.
da7bc1b include: in doc, remove article in front of "pointer" (Jonas Nick)
aa3dd52 include: make doc about ctx more consistent (Jonas Nick)
e3f6900 include: remove obvious "cannot be NULL" doc (Jonas Nick)

Pull request description:

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

Tree-SHA512: 809f312fa0cd1e9502ac79b8a1c502b87e6dfc2db8ad6bbd96d7ddbdaadad0c3b6110fa704b770c353cd34d5bf5547541cbb5f2779425d7419b584e721c854c2
…k1_ge_storage) == 64 code path

ba5d72d assumptions: Use new STATIC_ASSERT macro (Tim Ruffing)
e53c2d9 Require that sizeof(secp256k1_ge_storage) == 64 (Tim Ruffing)
d0ba2ab util: Add STATIC_ASSERT macro (Tim Ruffing)

Pull request description:

  This gets rid of an untested code path. Resolves bitcoin-core/secp256k1#1352.

  This is a bit opinionated in the sense that it adds a static assertion where it's needed in `secp256k1_pubkey_save` and `secp256k1_pubkey_load`. I think this is justified in this case. It helps the reviewer verify that these functions are correct.

  See individual commit messages.

ACKs for top commit:
  sipa:
    utACK ba5d72d
  jonasnick:
    ACK ba5d72d

Tree-SHA512: 2553c0610b62bcda6d4ef26eb26b5b2e07acf723bcd299691a2d02da57af22b8763f63c2d4adb17d30de8825b6157be6e4f0398147854fbabdf8b865fb0b5c88
b37fdb2 check-abi: Minor UI improvements (Tim Ruffing)
ad5f589 check-abi: Default to HEAD for new version (Tim Ruffing)
9fb7e2f release process: Style and formatting nits (Tim Ruffing)
e7053d0 release process: Add email step (Tim Ruffing)
429d21d release process: Run sanity checks on release PR (Tim Ruffing)

Pull request description:

ACKs for top commit:
  hebasto:
    ACK b37fdb2.
  jonasnick:
    ACK b37fdb2

Tree-SHA512: 6e18a5b897d29a3dd3a73ba81623dd91c04fa6730fb56374b924dc84baaec8c55d0c689ee1a41dab9a03ccd566082fc59ffb5d68cafd536a136fc7aaac2d8ef5
This also makes the order in which module options are processed
consistent between CMake and autotools (the reverse order of the listing
printed to stdout).
… dependencies

e682267 build: Error if required module explicitly off (Tim Ruffing)
89ec583 build: Clean up handling of module dependencies (Tim Ruffing)

Pull request description:

  This is a cleanup which makes it easier to add further modules with dependencies, e.g., in #1452. The diff looks larger than it is because I also reordered the modules and made the order consistent between CMake and autotools.

  (We noticed that the current logic could be improved in BlockstreamResearch#275.)

ACKs for top commit:
  jonasnick:
    ACK e682267
  hebasto:
    ACK e682267.

Tree-SHA512: 040e791e5b5b9b8845a39632633a45ca759391455910bdefba2b7b77c6340e65df6eda18199ae2ad65c30ee2fc6630471437aec143c26fe09ae4c11409a37622
… configure option

42f8c51 cmake: Add `SECP256K1_LATE_CFLAGS` configure option (Hennadii Stepanov)

Pull request description:

  This PR enables users to override compiler flags that have been set by the CMake-based build system, such as warning flags.

  The Autotools-based build system has the same feature out-of-the-box.

  See more details [here](bitcoin-core/secp256k1#1235 (comment)).

  Here are some examples of the new option usage:
  ```
  cmake -S . -B build -DSECP256K1_LATE_CFLAGS="-Wno-extra -Wlong-long"
  ```

  ```
  cmake -S . -B build -DSECP256K1_BUILD_EXAMPLES=ON -DSECP256K1_LATE_CFLAGS=-O1
  cmake --build build
  ...
  In function ‘secp256k1_ecmult_strauss_wnaf’,
      inlined from ‘secp256k1_ecmult’ at /home/hebasto/git/secp256k1/src/ecmult_impl.h:353:5:
  /home/hebasto/git/secp256k1/src/ecmult_impl.h:291:5: warning: ‘aux’ may be used uninitialized [-Wmaybe-uninitialized]
    291 |     secp256k1_ge_table_set_globalz(ECMULT_TABLE_SIZE(WINDOW_A) * no, state->pre_a, state->aux);
        |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:29:
  /home/hebasto/git/secp256k1/src/ecmult_impl.h: In function ‘secp256k1_ecmult’:
  /home/hebasto/git/secp256k1/src/group_impl.h:174:13: note: by argument 3 of type ‘const secp256k1_fe *’ to ‘secp256k1_ge_table_set_globalz’ declared here
    174 | static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const secp256k1_fe *zr) {
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:30:
  /home/hebasto/git/secp256k1/src/ecmult_impl.h:345:18: note: ‘aux’ declared here
    345 |     secp256k1_fe aux[ECMULT_TABLE_SIZE(WINDOW_A)];
        |                  ^~~
  ...
  ```

  Please note that in the last case providing `env CFLAGS=-O1` or `-DCMAKE_C_FLAGS=-O1` won't work.

ACKs for top commit:
  real-or-random:
    ACK 42f8c51

Tree-SHA512: 2b152e420a4a8ffd5f67857de03ae5ba9f2223e535ac01a867c1025e0619180d8255fdd1e5fb8279b290f0a1c96bcc874043ef968fcd99b1ff4e13041a91b1e1
@jonasnick
Copy link
Contributor

While we're at it: in bppp_norm_product_impl.h there's code prefaced with the comment /* res1 and res2 should be equal. Could not find a simpler way to compare them */. We can replace the code with secp256k1_gej_eq_var.

@real-or-random
Copy link
Collaborator Author

That all makes sense, I'll address it next week.

@real-or-random
Copy link
Collaborator Author

I've addressed all your comments.

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.

ACK b673a43

@jonasnick jonasnick merged commit b5a6812 into BlockstreamResearch:master Jan 23, 2024
107 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants