Upstream PRs 879, 959, 955, 944, 951, 960, 844, 963, 965 #140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Doing so could be considered UB in a strict reading of the standard. Avoid it.
This avoids having to remove trailing NUL bytes in the nonce function
Bitcoin Core's `configure` script uses `AC_CHECK_PROG` to find brew in the `PATH` [1]. If found, this macro will set `BREW=brew`. When building with dependencies however the `BREW` variable is set to `no` on macOS via `depends/<host_prefix>/share/config.site` [2] and this overrides `AC_CHECK_PROG` results [3]. Ideally, secp256k1's `configure` script should follow the same logic but this is not what happens because secp256k1's `configure` uses `AC_PATH_PROG` instead which respects preset variable values (in this case for variable `BREW`) only if they are a valid path (i.e., they match `[\\/*] | ?:[\\/]*` [4]), and `no` is not a path. This commit changes `AC_PATH_PROG` to `AC_CHECK_PROG` to be consistent with Core's `AC_CHECK_PROG`. Both of these macros are supposed to find executables in the `PATH` but the difference is that former is supposed to return the full path whereas the latter is supposed to find only the program. As a result, the latter will accept even non-paths `no` as an override. Not knowing the full path is not an issue for the `configure` script because it will only execute `BREW` immediately afterwards, which works fine without the full path. (In particular, `PATH` cannot have changed in between [5].) [1] https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L684 [2] https://github.com/bitcoin/bitcoin/blob/master/depends/config.site.in#L73-L76 [3] https://github.com/autotools-mirror/autoconf/blob/6d38e9fa2b39b3c3a8e4d6d7da38c59909d3f39d/lib/autoconf/programs.m4#L47 [4] https://github.com/autotools-mirror/autoconf/blob/6d38e9fa2b39b3c3a8e4d6d7da38c59909d3f39d/lib/autoconf/programs.m4#L127 [5] [3ab1178](bitcoin-core/secp256k1@3ab1178)
9570f67 Avoid passing out-of-bound pointers to 0-size memcpy (Pieter Wuille) Pull request description: Doing so could be considered UB in a pedantic interpretation of the standard. Avoid it. Closes #876. ACKs for top commit: practicalswift: cr ACK 9570f67: patch looks correct real-or-random: ACK 9570f67 Tree-SHA512: f991462d72e39f14e609021b8427c2e6756009bc8cd21efca2da46ec9410250725a4fed662df20fcdcfd10a4dc59038f13e8c166362b2eadde4366586b9ca72b
This makes the default sign function easier to use while allowing more granular control through sign_custom. Tests for sign_custom follow in a later commit.
Gives users the ability to hash messages to 32 byte before they are signed while allowing efficient domain separation through the tag.
Varlen message support for the default sign function comes from recommending tagged_sha256. sign_custom on the other hand gets the ability to directly sign message of any length. This also implies signing and verification support for the empty message (NULL) with msglen 0. Tests for variable lengths follow in a later commit.
This simplifies the interface of sign_custom and allows adding more parameters later in a backward compatible way.
Function `test_inverse_scalar` contains:
(var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&l, x); /* l = 1/x */
The two sides of the condition are the same function. This seems to be
an error, as there also exists a non-var function, named
`secp256k1_scalar_inverse`.
Make `test_inverse_scalar` use this other function when `var` is false.
This issue was found using clang's static analyzer, which reported a
"Logic error: Identical expressions in conditional expression" (with
checker `alpha.core.IdenticalExpr`).
…ar inverse 41ed139 tests: really test the non-var scalar inverse (Nicolas Iooss) Pull request description: ACKs for top commit: real-or-random: ACK 41ed139 jonasnick: ACK 41ed139 Tree-SHA512: d501300fea3f24af669556317ca899f6d184a2b1b64a3705417fce7c028288348555942604672eafa3ec59884849655a55cd9aacdd9ca8e34edf21b081702438
bdf19f1 Add random field multiply/square tests (Pieter Wuille) Pull request description: ACKs for top commit: real-or-random: ACK bitcoin-core/secp256k1@bdf19f1 jonasnick: ACK bdf19f1 Tree-SHA512: e78ce25f5440e87ad2cad0d4a87e5d95c983bc0be3a3e53d97f9cf6d8b3c3db9a830cb5f2f8c62f2f6dc9c6703c2a507cc23fa18d60bb624716e024539db5c21
Fixes one of the items in #923, namely the warnings of the form
'_putenv' redeclared without dllimport attribute:
previous dllimport ignored [-Wattributes]
This also cleans up the way we add CFLAGS, in particular flags enabling
warnings. Now we perform some more fine-grained checking for flag
support, which is not strictly necessary but the changes also help to
document autoconf.ac.
This also tidies the list of environment variables in .cirrus.yml.
0302138 ci: Make compiler warning into errors on CI (Tim Ruffing) b924e1e build: Ensure that configure's compile checks default to -O2 (Tim Ruffing) 7939cd5 build: List *CPPFLAGS before *CFLAGS like on the compiler command line (Tim Ruffing) 595e8a3 build: Enable -Wcast-align=strict warning (Tim Ruffing) 0725626 build: Use own variable SECP_CFLAGS instead of touching user CFLAGS (Tim Ruffing) Pull request description: ACKs for top commit: jonasnick: ACK 0302138 Tree-SHA512: 619eb6b512ae0eb8c51134f5bb1b7bc7a397321dc51073ae3117f9433505ec19b407518b47a181163e1a841216b20487c7a50c6f5045faffa5cfa7fad0b8c906
…AC_CHECK_PROG a4642fa configure: replace AC_PATH_PROG to AC_CHECK_PROG (UdjinM6) Pull request description: ACKs for top commit: real-or-random: ACK a4642fa jonasnick: utACK a4642fa Tree-SHA512: 55a431633ca45ea78be3887cda2e94f6ec9e8a937bc60cf04f14d7e3be11acb7ee861bd356070e3b1f6ccdeff28c6f9ab7048a58f920681c09fe3a976621a187
If `secp256k1_ecdsa_sign` fails, the signature which is then loaded by `secp256k1_ecdsa_signature_load` is garbage. Exit early with an error when this occurs.
a1ee83c tests_exhaustive: check the result of secp256k1_ecdsa_sign (Nicolas Iooss) Pull request description: Hello, In `test_exhaustive_sign`, if `secp256k1_ecdsa_sign` fails, the signature which is then loaded by `secp256k1_ecdsa_signature_load` is garbage. Exit early with an error when this occurs. By the way, I am wondering whether attribute `SECP256K1_WARN_UNUSED_RESULT` should be added to function `secp256k1_ecdsa_sign`: as (according to the documentation of this function) the nonce generation function may fail, it seems to be a good idea to force callers to check the value returned by this function. What do you think about this? ACKs for top commit: sipa: ACK a1ee83c real-or-random: utACK a1ee83c Tree-SHA512: d8c186afecbd95522e909c269255e8879695bf9df2de91f0f9303e575e18f03cafc66683d863e6cf9892fe61b668eab00d586861c39013292b71484a962f846d
5f6ceaf schnorrsig: allow setting MSGLEN != 32 in benchmark (Jonas Nick) fdd06b7 schnorrsig: add tests for sign_custom and varlen msg verification (Jonas Nick) d8d806a schnorrsig: add extra parameter struct for sign_custom (Jonas Nick) a0c3fc1 schnorrsig: allow signing and verification of variable length msgs (Jonas Nick) 5a8e499 Add secp256k1_tagged_sha256 as defined in BIP-340 (Jonas Nick) b6c0b72 schnorrsig: remove noncefp args from sign; add sign_custom function (Jonas Nick) 442cee5 schnorrsig: add algolen argument to nonce_function_hardened (Jonas Nick) df3bfa1 schnorrsig: clarify result of calling nonce_function_bip340 without data (Jonas Nick) 99e8614 README: mention schnorrsig module (Jonas Nick) Pull request description: This is a work in progress because I wanted to put this up for discussion before writing tests. It addresses the TODOs that didn't make it in the schnorrsig PR and changes the APIs of `schnorrsig_sign`, `schnorrsig_verify` and `hardened_nonce_function`. - Ideally, the new `aux_rand32` argument for `sign` would be const, but didn't find a solution I was happy with. - Support for variable length message signing and verification supports the [suggested BIP amendment](sipa/bips#207 (comment)) for such messages. - ~~`sign_custom` with its opaque config object allows adding more arguments later without having to change the API again. Perhaps there are other sensible customization options, but I'm thinking of [sign-to-contract/covert-channel](bitcoin-core/secp256k1#590) in particular. It would require adding the fields `unsigned char *s2c_data32` and `secp256k1_s2c_opening *s2c_opening` to the config struct. The former is the data to commit to and the latter is written to by `sign_custom`.~~ (EDIT: see below) ACKs for top commit: ariard: utACK 5f6ceaf LLFourn: utACK 5f6ceaf Tree-SHA512: cf1716dddf4f29bcacf542ed22622a817d0ec9c20d0592333cb7e6105902c77d819952e776b9407fae1333cbd03d63fded492d3a5df7769dcc5b450d91bb4761
unsigned char foo[4] = "abcd" is not valid C++ because the string literal "abcd" does not fit into foo due to the terminating NUL character. This is valid in C, it will just omit the NUL character. Fixes #962.
This is forbidden in C++.
C++ does not allow initialization with string literals but we do it in other places and -fpermissive will convince g++ to compile.
90e8344 ci: Add C++ test (Tim Ruffing) f698caa Use unsigned char consistently for byte arrays (Tim Ruffing) b5b8e7b Don't declare constants twice (Tim Ruffing) 769528f Don't use string literals for char arrays without NUL termination (Tim Ruffing) 2cc3cfa Fix -Wmissing-braces warning in clang (Tim Ruffing) Pull request description: ACKs for top commit: jonasnick: ACK 90e8344 Tree-SHA512: c26ba3db7514399c502f6c5c6f6ce6703459d83d831765042e331b051aeee282641197c3ae881c614f51ca714a818c5528410d288aadbd3e92361c1e9c129afe
aeece44 gen_context: Don't use any ASM (Tim Ruffing) Pull request description: See bitcoin/bitcoin#22441 , we need to wait for the testing results there. ACKs for top commit: sipa: utACK aeece44 jonasnick: ACK aeece44 Tree-SHA512: 52ff90f3dedda90124140de1c2c1c065a2f9374930d6b988d35c37f5eeae97f7d557b7ab0cf99d22add5a76ff8a3e06226572e43949e12d1048cb323d1b3d92b
apoelstra
changed the title
apoelstra
force-pushed
the
2021-07--resync
branch
from
8269dec
to
2979e4d
Compare
sanket1729
approved these changes
Jul 27, 2021
There was a problem hiding this comment.
Ran the scirpt. Verified and reviewed the diff. Everything makes sense, but
the resolution of this diff. This is correct, but out of scope for me to understand and review as I don't know the features supported by secp256k1-zkp
ACK 6ad66de
++<<<<<<< HEAD
+ ECDSA_S2C: no
+ GENERATOR: no
+ RANGEPROOF: no
+ WHITELIST: no
+ MUSIG: no
+ ECDSAADAPTOR: no
+ EXPERIMENTAL: no
+ CTIMETEST: yes
+ BENCH: yes
++=======
+ ### test options
++>>>>>>> be8d9c2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[bitcoin-core/secp256k1#879]: Avoid passing out-of-bound pointers to 0-size memcpy
[bitcoin-core/secp256k1#959]: tests: really test the non-var scalar inverse
[bitcoin-core/secp256k1#955]: Add random field multiply/square tests
[bitcoin-core/secp256k1#944]: Various improvements related to CFLAGS
[bitcoin-core/secp256k1#951]: configure: replace AC_PATH_PROG to AC_CHECK_PROG
[bitcoin-core/secp256k1#960]: tests_exhaustive: check the result of secp256k1_ecdsa_sign
[bitcoin-core/secp256k1#844]: schnorrsig API overhaul
[bitcoin-core/secp256k1#963]: "Schnorrsig API overhaul" fixups
[bitcoin-core/secp256k1#965]: gen_context: Don't use any ASM
This PR can be recreated with
./contrib/sync-upstream.sh range be8d9c262f46309d9b4165b0498b71d704aba8fe.