0e5ea6220707d9c96e06efc43bce3d5a3b3a06f2 CONTRIBUTING: add some coding and style conventions (Jonas Nick)
1a432cb98220f29ac47639d30a6dbb3aa679a441 README: update first sentence (Jonas Nick)
0922a047fb2a225fd89802bbd6f2d0919cd50bca docs: move coverage report instructions to CONTRIBUTING (Jonas Nick)
76880e40151ddb18d0cd0549502d5ade95f501d6 Add CONTRIBUTING.md including scope and guidelines for new code (Jonas Nick)
Pull request description:
Following offline discussions, this PR documents the scope of the library and the requirements for adding new modules. I think this fixes most of #997. It also updates the README very slightly.
In addition, I added some coding conventions that I remembered explaining to new contributors in the past year. Even though it's far from exhaustive, I think this is an easy improvement to the CONTRIBUTING.md. Feel free to suggest more conventions.
ACKs for top commit:
sipa:
ACK 0e5ea6220707d9c96e06efc43bce3d5a3b3a06f2
real-or-random:
ACK 0e5ea6220707d9c96e06efc43bce3d5a3b3a06f2
Tree-SHA512: ffdbab22982fd632de92e81bd135f141ac86e24cc0dcfc0e1ae12b0d2a2e4f91377ab2c0cc440cb919889eaed8bfc1447b880fa1430fd771b956f2af0fe3766e
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.
This removes the ununsed `obj` directory. It also suggests in the README
to create the "coverage" files in a separate directory and adds the
coverage files to .gitignore.
readme: Improve instructions for coverage reports
* 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>
bde2a32286c697dd1056aa3eb1ea2a5353f0bede 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 bde2a32286c697dd1056aa3eb1ea2a5353f0bede 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`.
78c38363412db3ea1cd1f0cc42dd1624c078ee32 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 78c38363412db3ea1cd1f0cc42dd1624c078ee32 I looked at the diff and verified my fingerprint
Tree-SHA512: 53a989615665cf8cf0c6a70d3bc2c4b71b68178cae40b2a7881aa9eba24732d126ba1e258a9fc127c69b47bb3025943097300cfcbbe18736cbf92ff4f3a901e0
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.
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
