4b48a43106 doc: document the length requirements of output parameter. (Rusty Russell)
Pull request description:
It's subtle, since it is actually only touched by hashfp (though
we assert it's non-NULL), but give explicit advice in the default
case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ACKs for top commit:
jonasnick:
ACK 4b48a43106
real-or-random:
ACK 4b48a43106 diff inspection
Tree-SHA512: d6bedb495e46b27ac9b558e77d814884d782ea78569a2296688eccf374bc880d13846546ad449c2a677865cf6ed56fcbc8be58c21f9daca5084831074e20d769
642cd062bd Remove Java Native Interface (Jonas Nick)
Pull request description:
This was discussed in #508. The main reasons are that the existing Java Native Interface (JNI) bindings would need way more work to remain useful to Java developers but the maintainers and regular contributors of libsecp are not very familiar with Java (and evidently are motivated enough to improve the situation). We don't know who relies on these bindings with the exception of ACINQ who have their own fork at https://github.com/ACINQ/secp256k1/tree/jni-embed/src/java (@sstone). Bitcoinj can optionally use the libsecp bindings.
Ideally, there would be a separate repository owned by Java developers with just the bindings. Until this exists, Java developers relying on libsecp can use ACINQs fork or an older commit of libsecp.
ACKs for top commit:
real-or-random:
ACK 642cd062bd I read the diff
real-or-random:
ACK 642cd062bd I read the diff, and I verified that the diff to 7d9066a66c0f13cabb0c4f71aca30edd3494f0d5, which has been ACKed by sipa, is only the additonal removal of ax_jni_include_dir.m4
Tree-SHA512: 9e573f2b01897bd5f301707062b41de53424517b537ce0834d9049d003cfd73fa1bcc024b543256016e4c9a1126f7c7fef559b84dc4914083b5a2d0ad5e57ea8
It's subtle, since it is actually only touched by hashfp (though
we assert it's non-NULL), but give explicit advice in the default
case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
acb7f97eb8 README: add a section for test coverage (Marko Bencun)
Pull request description:
It is a hassle to figure out the exact commands to create a good
report.
ACKs for top commit:
real-or-random:
ACK acb7f97eb8
sipa:
ACK acb7f97eb8
Tree-SHA512: d39f3e0b289229b2ce085406f6d716fdd54038df9ee5273a18a05140d1eddd4149149e881cc7a13f2126347217b9c56a0c12adf558c49879c5f556695242afc6
d567b779fe Clarify comments about use of rzr on ge functions and abs function. (Gregory Maxwell)
2241ae6d14 Remove secret-dependant non-constant time operation in ecmult_const. (Gregory Maxwell)
Pull request description:
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
ACKs for top commit:
real-or-random:
ACK d567b779fe I read the diff carefully and tested the code with ECDH enabled and various settings, also on valgrind
sipa:
ACK d567b779fe
Tree-SHA512: f00a921dcc6cc024cfb3ac1a34c1be619b96f1f17ec0ee0f3ff4ea02035ee288e55469491ed3183e2c4e5560cc068c10aafb657dff95a610706e5b9a8cd13966
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
2e759ec753 Overhaul README.md (Tim Ruffing)
Pull request description:
* Update feature list
* Be more positive about the state and quality of the library
* Mention ECDSA key operations explicitly in short library description
* Say "secret key" instead of "private key"
cc @gmaxwell who suggested a similar wording for the disclaimer.
ACKs for top commit:
sipa:
ACK 2e759ec753
jonasnick:
ACK 2e759ec753
Tree-SHA512: 2e1c87e7fa28d9dab682af227f845e7d48ac79a9fbe10be47ae4567abc2e066ba2f852c000db7d697ece8e4bbeeb851ea647465f870ac29dc3654031bf15a1ad
* Update feature list
* Be more positive about the state and quality of the library
* Mention ECDSA key operations explicitly in short library description
* Say "secret key" instead of "private key
* Define "experimental"
Co-Authored-By: Gregory Maxwell <greg@xiph.org>
bde2a32286 Convert bench.h to fixed-point math (Wladimir J. van der Laan)
Pull request description:
Convert `bench.h` to fixed-point math, removing all use of float math from the repository:
- Use 64-bit integer microsecond timestamps
- Use decimal fixed-point math for formatting numbers
It turned out to be a little trickier than I expected because of formatting and rounding. But, output should be the same before and after.
I used the following to test the number formatting: https://gist.github.com/laanwj/f971bfbe018e39c19677a21ff954d0c7
ACKs for top commit:
real-or-random:
ACK bde2a32286 I've read the code in detail and I've tested it. I haven't explicitly tested the formatting function with known/hardcoded inputs.
Tree-SHA512: 41ab6024b88c65a4b194272097c70d527bedb396dc7ab9d3d93165f1a19d31092798370f66399443a8d5393d0a6dcf5825679de5a325550865cfdef3586bf64c
- Use 64-bit integer microsecond timestamps
- Use fixed-point math for formatting numbers
Then, remove "except in benchmarks" exception from `README.md`.
78c3836341 Add SECURITY.md (Jonas Nick)
Pull request description:
Fixes#646
WIP because the secp256k1-security@bitcoincore.org email address doesn't exist yet. But it seems like the right place for vulnerability reports. security@bitcoincore.org would have the downside that it perhaps reaches more people than necessary. Ideally secp256k1-security would just forward to the three maintainers listed in SECURITY.md. @sipa @apoelstra is it okay to put you there? Fwiw I'm opting out for now because three people should be enough.
@sipa do you know who to talk to about adding secp256k1-security@bitcoincore.org and the specifics about how it would work?
ACKs for top commit:
real-or-random:
ACK 78c3836341 I looked at the diff and verified my fingerprint
Tree-SHA512: 53a989615665cf8cf0c6a70d3bc2c4b71b68178cae40b2a7881aa9eba24732d126ba1e258a9fc127c69b47bb3025943097300cfcbbe18736cbf92ff4f3a901e0
a0771d1 Explicitly disable buffering for stderr in tests (Jonas Nick)
fb424fb Make travis show the ./tests seed by removing stdout buffering and always cat tests.log after a travis run. (Jonas Nick)
Pull request description:
…by removing stdout buffering and always cat tests.log after a travis run. Fixes#645.
I noticed that according to the [doc](https://www.gnu.org/software/automake/manual/html_node/Parallel-Test-Harness.html) tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call `fflush` twice.
Travis is instructed to always show the seed which seems helpful with `after_script` by `cat`ing `./tests.log`. In case the tests fail it looks like https://travis-ci.org/jonasnick/secp256k1/jobs/606446234.
ACKs for commit a0771d:
real-or-random:
ACK a0771d15e6 I looked at the diff and checked that it does not break the tests
Tree-SHA512: 3ba37c2d9169867112981bba3d56680000651ef22ef684c3703f26ed3f71bf415fb23875d30059c8247ea9520c9cfad2c9207badf1b33da8fa3b7b7235a8bf16
dd98cc988f travis: Added a valgrind test without endro and enabled recovery+ecdh (Elichai Turkel)
b4c1382a87 Add valgrind check to travis (Elichai Turkel)
Pull request description:
As discussed in https://github.com/bitcoin-core/secp256k1/pull/687
This adds valgrind check to the repo.
It doesn't run on recovery+ecdh because of the time.
No openssl because of uninitialized mem.
I debated between with and without ASM, but decided with ASM because it might be more fragile(?).
I wasn't sure if I should pass `-DVALGRIND` via `CFLAGS` or `CPPFLAGS`, it seems like because this is only C then there shouldn't even be `CPPFLAGS` but looks like we use `CPPFLAGS` in other places for the preprocessor definitions.
If people are worried about the time it takes we can mark it as `allow_failure` although I don't think it's a problem here because there's only a handful of PRs and they're usually open for weeks.
ACKs for top commit:
real-or-random:
ACK dd98cc988f I looked at the diff
jonasnick:
ACK dd98cc988f
Tree-SHA512: 72d7f1f4c8dd4c58501ac1003b28296d6fd140a8f7711e9e3b3c04a3fbce358ff1c89d2e1d1c5489d7668d3019981264c5cadecae3d9b48cd38c9463e287d8ad
362bb25608 Modified bench_scalar_split so it won't get optimized out (Elichai Turkel)
73a30c6b58 Added accumulators and checks on benchmarks so they won't get optimized out (Elichai Turkel)
Pull request description:
As asked https://github.com/bitcoin-core/secp256k1/pull/667#issuecomment-546885951 this is the parts of #667 that don't require an assembly memory fence.
I splitted them to 2 commits, one with obvious easy ones. and another that changes the logic a bit to achieve this (See https://github.com/bitcoin-core/secp256k1/pull/667#discussion_r337248398 )
ACKs for top commit:
jonasnick:
ACK 362bb256
real-or-random:
ACK 362bb25608 I read the diff and I ran the benchmarks
Tree-SHA512: d5e47f5d64c3b035155276f057671ceb7f5852f24c7102fee4d0141aabebf882039f3eae0d152bae89d0603bc09fa6ad9f7bc6b8c0f74a668ee252c727517804
5c5f71e Fix ASM setting in travis (Jonas Nick)
Pull request description:
Without this PR the `ASM` setting isn't taken into account in travis.
ACKs for commit 5c5f71:
real-or-random:
ACK 5c5f71eea5 I read the diff
Tree-SHA512: 741650e4b9163e0e7341fa59b9859da85d0e34fa59980e68eacf59388879281b640836532acb3d8121da18d8e75a7c2993defada6329df830a99472b71cc17fe
bae1bea3c4 Make no-float policy explicit (Tim Ruffing)
Pull request description:
We don't want floating types for various reasons, e.g.,
- Their representation and often their behavior is implementation-defined.
- Many targets don't support them.
Closes#683.
ACKs for top commit:
jonasnick:
ACK bae1bea3c4
Tree-SHA512: e0027d6dda1a3e4b7d146fd3bea04e05473e08e25c0d0730018768be00351dfcf51b87b47b9e27953a21d42e0621433f13cbe55e4c20a7f7086e0191dff607a6
We don't want floating types for various reasons, e.g.,
- Their representation and often their behavior is implementation-defined.
- Many targets don't support them.
b76142f Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var which was removed in 47045270fa (Jonas Nick)
Pull request description:
...which was removed in 47045270fa. h/t @roconnor-blockstream
ACKs for commit b76142:
Tree-SHA512: 05fcd7aa5d765f1f5d31b93d40c2621e1dd9674a0db136a1e1cb216d6c01f5be1580275700cbdc08feda8f165b3b349640472d0bdec770bebb23f952225e3f52
0d82732 Improve VERIFY_CHECK of overflow in secp256k1_scalar_cadd_bit. This added check ensures that any curve order overflow doesn't go undetected due a uint32_t overflow. (Russell O'Connor)
8fe63e5 Increase robustness against UB. Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour. While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands. (roconnor-blockstream)
Pull request description:
Avoid possible, but unlikely undefined behaviour in `scalar_low_impl`'s `secp256k1_scalar_cadd_bit`.
Thanks to elichai2 who noted that the literal `1` is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour.
Using the unsigned literal `1u` addresses the issue.
ACKs for commit 0d8273:
real-or-random:
ACK 0d82732a9a
jonasnick:
ACK 0d82732a9a
Tree-SHA512: 905be3b8b00aa5cc9bd6dabb543745119da8f34181d37765071f28abbc1d6ff3659e3f195b72c2f2d003006678823919668bc0d169ac8b8d4bcc5da671813c99
59782c68b4 Remove mention of ec_privkey_export because it doesn't exist (Jonas Nick)
Pull request description:
Fixes#663
There is `ec_privkey_export_der` but it takes `0` for uncompressed and not `SECP256K1_EC_UNCOMPRESSED` (which is `2`).
ACKs for top commit:
real-or-random:
ACK 59782c68b4
apoelstra:
utACK 59782c68b4
Tree-SHA512: 6167581df74264be576f921d04bb8e23e16fa3b823bac4b45299079ceee38d6c74dd14a55b7b976a2cee9bdbd74dd6e3b39c0482808c1b8e65c8c80743f113a2
dcb2e3b3ff variable signing precompute table (djb)
Pull request description:
This pull request gives an option to reduce the precomputed table size for the signing context (`ctx`) by setting `#define ECMULT_GEN_PREC_BITS [N_BITS]`.
Motivation: Per #251 and #254, the static table can be reduced to 64kB. However, this is still too big for some of my embedded applications. Setting `#define ECMULT_GEN_PREC_BITS 2` produces a 32kB table at a tradeoff of about 75% of the signing speed. Not defining this value will default to the existing implementation of 4 bits. Statistics:
```
ECMULT_GEN_PREC_BITS = 1
Precomputed table size: 32kB
./bench_sign
ecdsa_sign: min 195us / avg 200us / max 212us
ECMULT_GEN_PREC_BITS = 2
Precomputed table size: 32kB
./bench_sign
ecdsa_sign: min 119us / avg 126us / max 134us
ECMULT_GEN_PREC_BITS = 4 (default)
Precomputed table size: 64kB
./bench_sign
ecdsa_sign: min 83.5us / avg 89.6us / max 95.3us
ECMULT_GEN_PREC_BITS = 8
Precomputed table size: 512kB
./bench_sign
ecdsa_sign: min 96.4us / avg 99.4us / max 104us
```
Only values of 2 and 4 make sense. 8 bits causes a larger table size with no increase in speed. 1 bit runs, actually, but does not reduce table size and is slower than 2 bits.
ACKs for top commit:
real-or-random:
ACK dcb2e3b3ff verified that all changes to the previous ACKed 1d26b27ac90092306bfbc9cdd5123e8a5035202a were due to the rebase
jonasnick:
ACK dcb2e3b3ff read the code and tested various configurations with valgrind
Tree-SHA512: ed6f68ca23ffdc4b59d51525336b34b25521233537edbc74d32dfb3eafd8196419be17f01cbf10bd8d87ce745ce143085abc6034727f742163f7e5f13f26f56e
make ECMULT_GEN_PREC_BITS configurable
ecmult_static_context.h: add compile time config assertion (#3) - Prevents accidentally using a file which was generated with a
different configuration.
README: mention valgrind issue
With --with-ecmult-gen-precision=8, valgrind needs a max stack size
adjustment to not run into a stack switching heuristic:
http://valgrind.org/docs/manual/manual-core.html
> -max-stackframe= [default: 2000000]
> The maximum size of a stack frame. If the stack pointer moves by more than this amount then Valgrind will assume that the program is switching to a different stack.
You may need to use this option if your program has large stack-allocated arrays.
basic-config: undef ECMULT_WINDOW_SIZE before (re-)defining it
a467047e11 Make ./configure string consistent (Tim Ruffing)
Pull request description:
This was forgotten in some PR rebase.
ACKs for top commit:
jonasnick:
ACK a467047e11
Tree-SHA512: 5aa67e886c165afa97a1e34ccfbd6bb0158ba4d4e5a4aacf6ac8b17ad9ee55132061957fd5ec383a79ad72ec7c92c745d7ad4fddca743b53e4b0e635616b29dc
b64a2e2597 Fix a nit in the recovery tests (Elichai Turkel)
Pull request description:
this signature is only valid under recid 1 not 0.
Source: https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/recovery/tests_impl.h#L247
(it passes only when the sig is parsed with recid 1)
ACKs for top commit:
real-or-random:
ACK b64a2e2597 I only looked at the diff
jonasnick:
ACK b64a2e2597 read the code
Tree-SHA512: 8e6744fe87c4078181dd1b334641784bf4fee37eb87346ecf8149482a9ea2c321bbe068e6a9199d836430b54b73848d94473a9aa6b59b4a68921a6321f449736
a11c76c59a secp256k1/src/tests.c: Properly handle sscanf return value (Mustapha Abiola)
Pull request description:
This pull request fixes a bug which allows the `sh` variable to be used uninitialised
when sscanf(3) returns EOF.
Signed-off-by: Mustapha Abiola <mustapha@trilemma.net>
ACKs for top commit:
sipa:
ACK a11c76c59a.
practicalswift:
utACK a11c76c59a
real-or-random:
ACK a11c76c59a I looked at the code
Tree-SHA512: fd9660a18e39ecf9366db94ccbcec2682b020223f4f982a4356ddf56c2fbdafa5edcd830db37be12b661c1ec0b15c57b9f34ba59ef4460187c9c2478376fbc88
Tim Ruffing
Rusty Russell
Jonas Nick
Marko Bencun
Pieter Wuille
Gregory Maxwell
Wladimir J. van der Laan
Elichai Turkel
djb