not-empty.
Before this, setting `VALGRIND=wat` was considered as true, and to make it
evaluate as false you had to unset the variable `VALGRIND=` but not it
checks if `VALGRIND=yes` and if it's not `yes` then it's evaluated to
false
18d36327fddad18ba81af2cf7fe6c8a16952dc22 secp256k1_gej_double_nonzero supports infinity (Pieter Wuille)
Pull request description:
Our existing function `secp256k1_gej_double_nonzero` actually supports infinity if only it wouldn't check that the input isn't infinity.
Drop the check, rename it to `secp256k1_gej_double`, and adapt the tests.
ACKs for top commit:
real-or-random:
ACK 18d36327fddad18ba81af2cf7fe6c8a16952dc22 I looked at the diff and ran tests locally
gmaxwell:
ACK 18d36327fddad18ba81af2cf7fe6c8a16952dc22
Tree-SHA512: 79dc42099c318f0bdfe7961495ab3fbbe87551c3cc373557a371914bb65638b129ddfd360e694959349f184e2d71a540abdbef04211e7eb70ee17b691632b915
When $USE_HOST or $EXTRAFLAGS are empty, we pass (due to quoting) an
empty string as a parameter to ./configure, which then believes we want
to use a deprecated syntax for specifing a host or a target and yells at us:
> configure: WARNING: you should use --build, --host, --target
The fixes are:
- $EXTRAFLAGS could contain multiple flags and should not be quoted at all.
- We can get rid of $USE_HOST by specifying --host="$HOST" directly.
67a429f31fd3d1b37c5365cc58b70588b8645d62 Suppress a harmless variable-time optimization by clang in _int_cmov (Tim Ruffing)
5b196338f0c8dc07bf0eece37b46d8686c4da3ce Remove redundant "? 1 : 0" after comparisons in scalar code (Tim Ruffing)
Pull request description:
Attempt at resolving #771 .
This surprisingly seems to improve the situation at least for the compilers available on godbolt.
ACKs for top commit:
gmaxwell:
ACK 67a429f31fd3d1b37c5365cc58b70588b8645d62
elichai:
tACK 67a429f31fd3d1b37c5365cc58b70588b8645d62
Tree-SHA512: ee8b0c86831ec8c3d5a9abcad773ed8a0f267e5c47012e4e1423b10a64c26b4cf6e3c466c3df765ba7e636787a3fe134d633926d67b599287f12c51be924f478
2e1b9e0458317d03b682c1f5dd63aedb52c86b04 tests: Abort if malloc() fails during context cloning tests (Tim Ruffing)
Pull request description:
Found by the clang static analyzer.
This is the worst true positive that it found. I feel somewhat proud.
ACKs for top commit:
elichai:
tACK 2e1b9e0458317d03b682c1f5dd63aedb52c86b04
Tree-SHA512: bf9a3b6c2b8beaafd230ece00a9a69dd884a35b6d2243502ebfded3f77a454e80ef922791bd48c17aa4814a275550957071c045912080a616dd5ed704a70aab7
37dba329c6cb0f7a4228a11dc26aa3a342a3a5d0 Remove unnecessary sign variable from wnaf_const (Jonas Nick)
6bb0b77e158fc2f9e56e4b65b08bcb660d4c588b Fix test_constant_wnaf for -1 and add a test for it. (Jonas Nick)
Pull request description:
There currently is a single branch in the `ecmul_const` function that is not being exercised by the tests. This branch is unreachable and therefore I'm suggesting to remove it.
For your convenience the paper the wnaf algorithm can be found [here (The Width-w NAF Method Provides Small Memory and Fast Elliptic Scalar Multiplications Secure against Side Channel Attacks)](http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.563.1267&rep=rep1&type=pdf). Similarly, unless I'm missing something important, I don't see how their algorithm needs to consider `sign(u[i-1])` unless `d` can be negative - which doesn't make much sense to me either.
ACKs for top commit:
real-or-random:
ACK 37dba329c6cb0f7a4228a11dc26aa3a342a3a5d0 I verified the correctness of the change and claimed invariant by manual inspection. I tested the code, both with 32bit and 64bit scalars.
Tree-SHA512: 9db45f76bd881d00a81923b6d2ae1c3e0f49a82a5d55347f01e1ce4e924d9a3bf55483a0697f25039c327e33edca6796ba3205c068d9f2f99aa5d655e46b15be
1309c03c45beece646a7d21fdb6a0e3d38adee2b Fix some compile problems on weird/old compilers. (Gregory Maxwell)
Pull request description:
The visibility attribute is a GCC 4+ feature.
GCC 2.95 also warns about the unsigned/signed comparision.
ACKs for top commit:
real-or-random:
ACK 1309c03c45beece646a7d21fdb6a0e3d38adee2b I inspected the diff
Tree-SHA512: b5a5175416b67b2619f68ad82a208052ad678955e59c2f3457799abd1dd6fd817c40f6bc2941b2bda207c6f58ad0fbe46221a2f92b726e824702c4c0b177377c
f00d6575ca0dcca11e085b20afa4d73dc8742ddc remove dead store in ecdsa_signature_parse_der_lax (fanquake)
Pull request description:
ACKs for top commit:
elichai:
utACK f00d6575ca0dcca11e085b20afa4d73dc8742ddc, it does look like we don't use that assignment
jonasnick:
ACK f00d6575ca0dcca11e085b20afa4d73dc8742ddc
Tree-SHA512: 9aa54c901f299341c309411b0247720f5152a131dd346c19be7ee21865e3a822e8cf91b869e28ef6288adaf31660bc2e18874e304052468a9be6b7027674af30
2e7fc5b5372067ecd33b2304e8c88ed6de98ff13 Fix uninitialized variables in ecmult_multi test (Jonas Nick)
Pull request description:
Fixes#756
ACKs for top commit:
real-or-random:
ACK 2e7fc5b5372067ecd33b2304e8c88ed6de98ff13 I inspected the diff. I did not test it and I did not check whether if makes the warning go away
elichai:
tACK 2e7fc5b5372067ecd33b2304e8c88ed6de98ff13
Tree-SHA512: 674400134f5487236f5b6e8b3020b346d43662511628cdf6dd1bd7ba1de985bf93f5be11f5650f250ff37b5f87eb4b01d90ed53d41193c05a420d3f5a2d63470
28609507e70a172dd8f39de4aa55f851452fc0b4 Add tests for the cmov implementations (Elichai Turkel)
73596a85a2ab9c885e58a7a2a8876355a6ae68e4 Add ecdsa_sign_recoverable to the ctime tests (Elichai Turkel)
2876af4f8da952e39c06bc229d68cd4892ea2c85 Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery (Elichai Turkel)
Pull request description:
Hi,
The recovery module was overlooked in #708 and #710, so this adds it to the `valgrind_ctime_test` and replaces the secret dependent branching with the cmovs,
I created a new function `secp256k1_ecdsa_sign_inner` (feel free to bikeshed) which does the logic both for ecdsa_sign and for ecdsa_sign_recoverable, such that next time when things get changed/improved in ecdsa it will affect the recoverable signing too.
ACKs for top commit:
jonasnick:
ACK 28609507e70a172dd8f39de4aa55f851452fc0b4
real-or-random:
ACK 28609507e70a172dd8f39de4aa55f851452fc0b4 read the diff, tested with valgrind including ctime tests
Tree-SHA512: 4730301dcb62241d79f18eb8fed7e9ab0e20d1663a788832cb6cf4126baa7075807dc31896764b6f82d52742fdb636abc6b75e4344c6f117305904c628a5ad59
f79a7adcf555ccc78b591850ea15c64fbfbca152 Add valgrind uninit check to cmovs output (Elichai Turkel)
a39c2b09de304b8f24716b59219ae37c2538c242 Fixed UB(arithmetics on uninit values) in cmovs (Elichai Turkel)
Pull request description:
This should fix#753.
Used @peterdettman's solution here for the `ECMULT_CONST_TABLE_GET_GE` https://github.com/bitcoin-core/secp256k1/issues/753#issuecomment-631316091
and in ecdsa_sign I initialize `s` and `r` to a zero scalar.
The second commit adds a valgrind check to the cmovs that could've caught this (in ecdsa_sign, not in ecmult_const because there's a scalar clear there under `VERIFY_SETUP`)
ACKs for top commit:
sipa:
utACK f79a7adcf555ccc78b591850ea15c64fbfbca152
jonasnick:
ACK f79a7adcf555ccc78b591850ea15c64fbfbca152
real-or-random:
ACK f79a7adcf555ccc78b591850ea15c64fbfbca152
Tree-SHA512: 6fd7b7c84f392bda733a973f4dcfc12bf1478aac2591e2c87b69e637847d3b063c4243cc8feccaffc3a5824c18183a5e66bd4251c2322abaf63bb6439b38defe
5e8747ae2a0c915d079837d238f8b84841a4ce5c autoconf: Use ":" instead of "dnl" as a noop (Tim Ruffing)
Pull request description:
Fixes#424.
Top commit has no ACKs.
Tree-SHA512: a83664afbc6ca1254c4767161bfbec82f3489a8a248ba7a5a46ed9ec2a39232cf92f504accadd4dbb1a6ea4791dbf7f0e1f030e51f02f49eb9a38a2e509ee6c2
71757da5ccece100b1eca6c70b4d87e2542cff97 Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh (Elichai Turkel)
99bd661d71fe17be7b3de4707a0264c41a63ebe8 Replace travis_wait with a loop printing "\a" to stdout every minute (Elichai Turkel)
bc818b160cdaaccff2162206cc15915fa5f0cca8 Bump travis Ubuntu from xenial(16.04) to bionic(18.04) (Elichai Turkel)
0c5ff9066e6fa41b1fbd5d0b8c2f02e8a04e96ea Add macOS support to travis (Elichai Turkel)
b6807d91d83e9597ffecec999eb761b8571a1f26 Move travis script into a standalone sh file (Elichai Turkel)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 71757da5ccece100b1eca6c70b4d87e2542cff97 I inspected the diff
jonasnick:
ACK 71757da5ccece100b1eca6c70b4d87e2542cff97
Tree-SHA512: e8fab725ef5ed98c795f39d7f26b5d967a6bd730d40eb7d9793986858bf34770b0350c1b7b1d14ae608dfff9375a0750ec67c8e6d0d4b562ab917f5e645aa67b
7e3952ae82af2a98619de74614f2d3f0ff072941 Clarify documentation of tweak functions. (Jonas Nick)
89853a0f2e2b324bd32e78641aeaad6d1e427e81 Make tweak function documentation more consistent. (Jonas Nick)
41fc78560223aa035d9fe2cbeedb3ee632c740e2 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul (Jonas Nick)
22911ee6da7197c45226ab4bcd84b8c2773baf9f Rename private key to secret key in public API (with the exception of function names) (Jonas Nick)
5a73f14d6cfab043f94b2032d6958e15f84065fe Mention that value is unspecified for In/Out parameters if the function returns 0 (Jonas Nick)
f03df0e6d7d272808c9d27df40bb92716b341d03 Define valid ECDSA keys in the documentation of seckey_verify (Jonas Nick)
5894e1f1df74b23435cfd1e9a8b25f22ac631ebe Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul (Jonas Nick)
8f814cddb94f4018646569143d10ebbb98daa454 Add test for boundary conditions of scalar_set_b32 with respect to overflows (Jonas Nick)
3fec9826086aa45ebbac1ff6fc3bb7b25ca78b1d Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify (Jonas Nick)
9ab2cbe0ebf8dfafd5499ea7a79c35f0d0cfe5e2 Add scalar_set_b32_seckey which does the same as scalar_set_b32 and also returns whether it's a valid secret key (Jonas Nick)
Pull request description:
Fixes#671. Supersedes #668.
This PR unifies handling of invalid secret keys by introducing a new function `scalar_set_b32_secret` which returns false if the b32 overflows or is 0. By using this in `privkey_{negate, tweak_add, tweak_mul}` these function will now return 0 if the secret key is invalid which matches the behavior of `ecdsa_sign` and `pubkey_create`.
Instead of deciding whether to zeroize the secret key on failure, I only added documentation for now that the value is undefined on failure.
ACKs for top commit:
real-or-random:
ACK 7e3952ae82af2a98619de74614f2d3f0ff072941 I read the diff carefully and tested the changes
apoelstra:
ACK 7e3952ae82af2a98619de74614f2d3f0ff072941
Tree-SHA512: 8e9a66799cd3b6ec1c3acb731d6778035417e3dca9300d840e2437346ff0ac94f0c9be4de20aa2fac9bb4ae2f8a36d4e6a34795a640b9cfbfee8311decb102f0
Before, test_constant_wnaf used scalar_cadd_bit to correct for the skew. But
this function does not correctly deal with overflows which is why num = -1
couldn't be tested.
This commit also adds tests for 0, 1/2 and 1/2-1 as they are corner cases
in constant_wnaf.
37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4 Make ecdsa_sig_sign constant-time again after reverting 25e3cfb (Tim Ruffing)
93d343bfc5323e56f6a60cb41d60b96368cc09c7 Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign" (Tim Ruffing)
Pull request description:
ACKs for top commit:
elichai:
ACK 37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4 makes sense.
jonasnick:
ACK 37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4
Tree-SHA512: 82b5b8e29f48e84fd7a0681b62923d3bd87d724b38ef18e8c7969b0dcc5a405ebb26c14b5c5f4c7ba0ccabd152d1531d217809d1daf40872fe0c1e079b55c64b
4e284655d99c8f0ee030aa29bcd036e5e5bd2a40 Fix typo in ecmult_const_impl.h (f-daniel)
Pull request description:
Fix small typo in the reference given for the wNAF method ( `secp256k1_wnaf_const`)
ACKs for top commit:
real-or-random:
ACK 4e284655d99c8f0ee030aa29bcd036e5e5bd2a40 trivial
Tree-SHA512: d6c3daa0384fc1bba36a46933641c97661b18e88c711343e2c8f91f39aa707eddd0ba77c8d5c43aaead883eeed7f4458ed1dec228d692d713572231aa6010fb0
ffef45c98a55934ab555ecf96f0b19d0e4cfe830 Make recovery/main_impl.h non-executable (Elichai Turkel)
Pull request description:
Opened it because of https://github.com/bitcoin/bitcoin/pull/18650
I assume it doesn't matter?
But because I'm not sure I preferred to open this then let the info go away in case someone thinks it does matter.
ACKs for top commit:
real-or-random:
ACK ffef45c98a55934ab555ecf96f0b19d0e4cfe830
Tree-SHA512: 381aed7f99fd739f4059b2e526ba9cd75b55b4fa86c9cc040fbf6b93055ce8558cc69c4ccf5d8a422b17022ca376cc9a608cf5af8d5841d62c5953f40825f5ff
3b7d26b23c9811deee244ede7bb344bb43544153 build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS (fanquake)
84b5fc5bc34cfb6e9df4f06b3b6fc98c3373dd0e build: fix OpenSSL EC detection on macOS (fanquake)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 3b7d26b23c9811deee244ede7bb344bb43544153 the diff looks good to me
jonasnick:
ACK 3b7d26b23c9811deee244ede7bb344bb43544153
Tree-SHA512: 381aed7f99fd739f4059b2e526ba9cd75b55b4fa86c9cc040fbf6b93055ce8558cc69c4ccf5d8a422b17022ca376cc9a608cf5af8d5841d62c5953f40825f5ff
This reverts commit 25e3cfbf9b52d2f5afa543f967a73aa8850d2038. The reverted
commit was probably based on the assumption that this is about the touched
checks cover the secret nonce k instead of r, which is the x-coord of the public
nonce. A signature with a zero r is invalid by the spec, so we should return 0
to make the caller retry with a different nonce. Overflow is not an issue.
Fixes#720.
