d216475205 test secp256k1_i128_to_i64 (Russell O'Connor)
4bc429019d Add a secp256k1_i128_to_u64 function. (Russell O'Connor)
Pull request description:
I wanted to experiment with what would be required to split up `secp256k1_i128_to_i64` between those cases when a signed 64 bit value is being demoted, versus an unsigned 64 bit value is being extracted from the lower bits, and this is the result.
I'm not sure this is a useful PR, so feel free to close it. However, since it is already written, I figured it is worth at least discussing.
ACKs for top commit:
sipa:
utACK d216475205
real-or-random:
ACK d216475205
Tree-SHA512: 41dbb1d33b3078bee8e71a838cfad6f1859c0bba602ae061259add8e9e8ea5aa482daa41de79dbd7433ddbef4a0bc52757f3c45d63acc9c0eb05aa3ca891b922
This change eases the use of alternate build systems by moving
the variables in `src/libsecp256k1-config.h` to compiler macros
for each invocation, preventing duplication of these variables
for each build system.
Co-authored-by: Ali Sherief <ali@notatether.com>
The vector has been taken from https://www.di-mgt.com.au/sha_testvectors.html.
It can be independently verified using the following Python code.
```
h = hashlib.sha256()
for i in range(1_000_000):
h.update(b'a')
print(h.hexdigest())
```
070e772211 Faster fixed-input ecmult tests (Pieter Wuille)
Pull request description:
Given how much #920 slowed down the tests with low iteration count, replace it with 3 different similar test:
* count >= 1: a test with 1024 multiplies that tests any pattern of 6 bits in windows not more than 20 bits wide
* count >= 3: a test with 2048 multiplies that tests any pattern of 8 consecutive bits
* count >= 35: the old test (which effectively tests all 2-bit patterns)
ACKs for top commit:
robot-dreams:
ACK 070e772211, the addition of the `CONDITIONAL_TEST` macro is nice.
real-or-random:
ACK 070e772211
Tree-SHA512: b4ccca42c71fcd1baa7143f73d1c3ac9d012c296485164a03341dbeee02e4ba9f7c7ad6b441923a5fe0286c97eff60815033adb4e1d30b3ef08bcb79590327ff
3d7cbafb5f tests: Fix test whose result is implementation-defined (Tim Ruffing)
Pull request description:
A compiler may add struct padding and fe_cmov is not guaranteed to
preserve it.
On the way, we restore the name of the function. It was mistakenly
renamed in 6173839c90 using
"search and replace".
ACKs for top commit:
robot-dreams:
ACK 3d7cbafb5f
sipa:
utACK 3d7cbafb5f
Tree-SHA512: f8bb643d4915e9ce9c4fe45b48a2878f6cf1f29e654be1c150cdf65c6959cf65f8491928cf098da5a01f1d488ba475914905ca96b232abed499eb6ed65e53fb8
A compiler may add struct padding and fe_cmov is not guaranteed to
preserve it.
On the way, we improve the identity check such that it covers the
VERIFY struct members.
7dfceceea6 build: Remove #undef hack for ASM in the precomputation programs (Tim Ruffing)
bb36fe9be0 ci: Test `make precomp` (Tim Ruffing)
d94a37a20c build: Remove CC_FOR_BUILD stuff (Tim Ruffing)
ad63bb4c29 build: Prebuild and distribute ecmult_gen table (Tim Ruffing)
ac49361ed0 prealloc: Get rid of manual memory management for prealloc contexts (Tim Ruffing)
6573c08f65 ecmult_gen: Tidy precomputed file and save space (Tim Ruffing)
5eba83f17c ecmult_gen: Precompute tables for all values of ECMULT_GEN_PREC_BITS (Tim Ruffing)
fdb33dd122 refactor: Make PREC_BITS a parameter of ecmult_gen_build_prec_table (Tim Ruffing)
a4875e30a6 refactor: Move default callbacks to util.h (Tim Ruffing)
4c94c55bce doc: Remove obsolete hint for valgrind stack size (Tim Ruffing)
5106226991 exhaustive_tests: Fix with ecmult_gen table with custom generator (Tim Ruffing)
e1a76530db refactor: Make generator a parameter of ecmult_gen_create_prec_table (Tim Ruffing)
9ad09f6911 refactor: Rename program that generates static ecmult_gen table (Tim Ruffing)
8ae18f1ab3 refactor: Rename file that contains static ecmult_gen table (Tim Ruffing)
00d2fa116e ecmult_gen: Make code consistent with comment (Tim Ruffing)
3b0c2185ea ecmult_gen: Simplify ecmult_gen context after making table static (Tim Ruffing)
e43ba02cfc refactor: Decouple table generation and ecmult_gen context (Tim Ruffing)
22dc2c0a0d ecmult_gen: Move table creation to new file and force static prec (Tim Ruffing)
Pull request description:
This resolves#893, resolves#692 (and also resolvesbitcoin/bitcoin#22854).
- [x] Extract table generation to separate function in separate file (to be used by generation script and exhaustive tests)
- [x] Tidy up
- [x] Remove code that deals with non-static tables
- [x] Make functions that need ecmult_gen not depend on signing context
- [x] Rename stuff to make it fit the new structure and consistent with how we hande verification tables (#956)
- [x] Fix exhaustive tests
- [x] Make table generation function take generator as input
- [x] Overwrite the static tables with a table with custom generator in exhaustive tests
- [x] Overhaul script that generates table files
- [x] Make table generation function take PREC_BITS as input (I have some code already, just not yet in this branch)
- [x] Change generation script to generate three tables (for all three values of ECMULT_GEN_PREC_BITS)
- [x] Ship pre-built tables
- [x] Add pregenerated table file to repo
- [x] Remove generation of table file from build process (like in #956)
- [x] Remove left-over stuff (e.g., detecting a compiler running on the build machine) from build system
- [x] Final cleanups (copyright headers, commit, messages, etc.)
- [ ] (separate PR:) Make sure link-time optimization remove corresponding static tables (and code) when no signing/verifcation function is called
- [ ] (separate PR:) Compile precomputation as a separate object file and link it (https://github.com/bitcoin-core/secp256k1/pull/988#issuecomment-977813538)
- [ ] (separate PR:) Document the backwards-compatible API changes made in this PR and in #956.
- [ ] Maybe deprecate the static context
ACKs for top commit:
sipa:
ACK 7dfceceea6
robot-dreams:
ACK 7dfceceea6 (based on range-diff between 56284c7d44c0ed46e636588bfbf6c403b7dfa6c1 and 7dfceceea6)
Tree-SHA512: 6efb3f36f05efe3b79bbd877881fe1409f71fd6488d24c811b2e77d9f053bed78670dd1dcbb42ad780458a51c4ffa36de9cd6567271b22041dc7a122ceb677c5
This is a backwards-compatible API change: Before this commit, a context
initialized for signing was required to call functions that rely on
ecmult_gen. After this commit, this is no longer necessary because the
static ecmult_gen table is always present. In practice this means that
the corresponding functions will just work instead of calling the
illegal callback when given a context which is not (officially)
initialized for signing.
This is in line with 6815761, which made the analogous change with
respect to ecmult and contexts initialized for signing. But as opposed
to 681571, which removed the ecmult context entirely, we cannot remove
the ecmult_gen context entirely because it is still used for random
blinding. Moreover, since the secp256k1_context_no_precomp context is
const and cannot meaningfully support random blinding, we refrain (for
now) from changing its API, i.e., the illegal callback will still be
called when trying to use ecmult_gen operations with the static
secp256k1_context_no_precomp context.
1. secp256k1_fe_verify is removed from tests since, it throws an error if VERIFY is not defined during compilation.
(Ex: ./configure --enable-coverage)
2. `secp256k1_fe_from_storage` calls `secp256k1_fe_verify` in the VERIFY build to check for invalid field element.
clang 7 to 11 (and maybe earlier versions) warn about recid being
potentially unitiliazed in "CHECK(recid >= 0 [...]", which was mitigated
in commit 3d2cf6c5bd by initializing recid
to make clang happy but VG_UNDEF'ing the variable after initializiation
in order to ensure valgrind's memcheck analysis will still be sound and
complain if recid is not actually written to when creating a signature.
However, it turns out that at least for binaries produced by clang 11
(but not clang 7), valgrind complains about a branch on unitialized data
in the recid variable in that line before *and* after the aforementioned
commit. While the complaint after the commit could be spurious (clang
knows that recid is initialized, so it's fine to access it even though
the access is stupid), the complaint before the commit indicates a real
problem: it might be the case that clang is performing a wrong
optimization that leads to a situation where recid is really not
guaranteed to be initialized when it's accessed. As a result, clang
warns about this and generates code that just accesses the variable.
I'm not going to bother with this further because this is fixed in
clang 12 and the problem is just in our test code, not in the tested
code.
This commit rewrites the code in a way that groups the signing together
with the CHECK such that it's very easy to figure out for clang that
recid will be initialized properly. This seems to circument the issue.
5f6ceafcfa schnorrsig: allow setting MSGLEN != 32 in benchmark (Jonas Nick)
fdd06b7967 schnorrsig: add tests for sign_custom and varlen msg verification (Jonas Nick)
d8d806aaf3 schnorrsig: add extra parameter struct for sign_custom (Jonas Nick)
a0c3fc177f schnorrsig: allow signing and verification of variable length msgs (Jonas Nick)
5a8e4991ad Add secp256k1_tagged_sha256 as defined in BIP-340 (Jonas Nick)
b6c0b72fb0 schnorrsig: remove noncefp args from sign; add sign_custom function (Jonas Nick)
442cee5baf schnorrsig: add algolen argument to nonce_function_hardened (Jonas Nick)
df3bfa12c3 schnorrsig: clarify result of calling nonce_function_bip340 without data (Jonas Nick)
99e8614812 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](https://github.com/sipa/bips/issues/207#issuecomment-673681901) 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](https://github.com/bitcoin-core/secp256k1/pull/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 5f6ceafcfa
Tree-SHA512: cf1716dddf4f29bcacf542ed22622a817d0ec9c20d0592333cb7e6105902c77d819952e776b9407fae1333cbd03d63fded492d3a5df7769dcc5b450d91bb4761
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`).
