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.
35+The easiest way to participate on IRC is with the web client, [web.libera.chat](https://web.libera.chat/#secp256k1).
36+Chat history logs can be found at https://gnusha.org/secp256k1/.
37+
38+## Contributor Workflow & Peer Review
39+
40+The Contributor Workflow & Peer Review in libsecp256k1 are similar to Bitcoin Core's workflow and review processes described in Core's [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md).
24+
25+These are not the only factors taken into account when considering to add new functionality.
26+The proposed new libsecp256k1 code must be of high quality, including API documentation and tests, as well as featuring a misuse-resistant API design.
27+
28+We recommend reaching out to other contributors (see [Communication Channels](#communication-channels)) and get feedback before implementing new functionality.
29+## Communication Channels
You can mention these points as well if they are relevant:
run_benchmark function available to evaluate the runtime performance of APIs, measuring min, avg, and max execution times (see src/bench.h)
@siv2r Thanks. I incorporated your suggestions, except:
API documentation is in their header files (see include)
I don’t think the CONTRIBUTING.md is the right place for that because this information is relevant to all users, not just developers of the library.
53+
54+#### Style Conventions
55+
56+- Commits should be atomic and diffs should be easy to read. For this reason, do not mix any formatting fixes or code moves with actual code changes. Make sure each individual commit is hygienic: that it builds successfully on its own without warnings, errors, regressions, or test failures.
57+- Use `void *ptr` instead of `void* ptr`.
58+- Avoid trailing whitespaces.
0- Avoid trailing whitespace.
59+
60+### Tests
61+
62+#### Coverage
63+
64+This library aims to have full coverage of the reachable lines and branches.
0This library aims to have full coverage of reachable lines and branches.
83+#### Exhaustive Tests
84+
85+There are tests of several functions in which a small group replaces secp256k1.
86+These tests are *exhaustive* since they provide all elements and scalars of the small group as input arguments (see [src/tests_exhaustive.c]).
87+
88+
4@@ -5,7 +5,7 @@ libsecp256k1
5 
6 [](https://web.libera.chat/#secp256k1)
7
8-Optimized C library for ECDSA signatures and secret/public key operations on curve secp256k1.
9+Optimized C library for digital signatures and other cryptographic schemes on curve secp256k1.
(I may be quite nitpicky here, but I think this line is rather important because it should become our new “short description” as often included by package managers and also presented at the right-hand side of the GitHub page.)
I think “other schemes” is a bit strange because “digital signatures” is not a scheme. It’s a “cryptographic primitive”. That word is a bit uncommon outside academia, but I think it’s okay.
I very much like the wording in the new file: “libsecp256k1 is a library for elliptic curve cryptography”. But yeah, it may still make sense to mention signatures. Here are a few suggestions:
Independently, I suggest dropping “Optimized”… It’s not wrong, but I think it made sense when the library was created because performance was by far the main feature. Today, it’s rather a combination of performance and assurance. We could call it “high-performance high-assurance” but I want others to judge this. (Also, it sounds like stolen from djb, and it just makes the description longer.)
“High-performance high-assurance” sounds pretty good in my opinion.
Okay, let’s take it. :D I think we can certainly claim it.
A final nit now that I see your new push: “on elliptic curve secp256k1” -> “on the secp256k1 elliptic curve” sounds a bit more natural
That makes it: High-performance high-assurance C library for digital signatures and other cryptographic primitives on the secp256k1 elliptic curve.
cc @sipa What do you think about this description line?
“on elliptic curve secp256k1” -> “on the secp256k1 elliptic curve” sounds a bit more natural
done
High-performance high-assurance C library for digital signatures and other cryptographic primitives on the secp256k1 elliptic curve.
ACK
42+
43+### Coding Conventions
44+
45+In addition, libsecp256k1 tries to maintain the following coding conventions:
46+
47+- No runtime heap allocation (e.g., no `malloc`).
25+These are not the only factors taken into account when considering to add new functionality.
26+The proposed new libsecp256k1 code must be of high quality, including API documentation and tests, as well as featuring a misuse-resistant API design.
27+
28+We recommend reaching out to other contributors (see [Communication Channels](#communication-channels)) and get feedback before implementing new functionality.
29+
30+## Communication Channels
7+
8+## Adding new functionality or modules
9+
10+The libsecp256k1 project welcomes contributions in the form of new functionality or modules, provided they are within the project's scope.
11+
12+It is the responsibility of the contributors to convince the maintainers that the proposed functionality should be merged.
style nit:
0It is the responsibility of the contributors to convince the maintainers that the proposed functionality suits the goals and the scope of the library.
13+Contributors are recommended to provide the following in addition to the new code:
14+
15+* **Specification:**
16+ A specification can help significantly in reviewing the new code as it provides documentation and context.
17+ It may justify various design decisions, give a motivation and outline security goals.
18+ If the specification contains pseudocode, a reference implementation or test vectors, these could be used to compare with the proposed libsecp256k1 code.
style nit:
0 If the specification contains pseudocode, a reference implementation or test vectors, these can be used to compare with the proposed libsecp256k1 code.
16+ A specification can help significantly in reviewing the new code as it provides documentation and context.
17+ It may justify various design decisions, give a motivation and outline security goals.
18+ If the specification contains pseudocode, a reference implementation or test vectors, these could be used to compare with the proposed libsecp256k1 code.
19+* **Security Arguments:**
20+ In addition to a defining the security goals, it should be argued that the new functionality meets these goals.
21+ Depending on the nature of the new functionality, a wide range of security arguments are acceptable, ranging from being 'obviously secure' to rigorous proofs of security.
nit:
0 Depending on the nature of the new functionality, a wide range of security arguments are acceptable, ranging from being "obviously secure" to rigorous proofs of security.
18+ If the specification contains pseudocode, a reference implementation or test vectors, these could be used to compare with the proposed libsecp256k1 code.
19+* **Security Arguments:**
20+ In addition to a defining the security goals, it should be argued that the new functionality meets these goals.
21+ Depending on the nature of the new functionality, a wide range of security arguments are acceptable, ranging from being 'obviously secure' to rigorous proofs of security.
22+* **Relevance Arguments:**
23+ The relevance of the new functionality for the Bitcoin space should be argued by outlining clear use cases.
style nit:
0 The relevance of the new functionality for the Bitcoin ecosystem should be argued by outlining clear use cases.
52+- Arguments of the publicly-facing API must have a specific order defined in [include/secp256k1.h](include/secp256k1.h).
53+
54+#### Style Conventions
55+
56+- Commits should be atomic and diffs should be easy to read. For this reason, do not mix any formatting fixes or code moves with actual code changes. Make sure each individual commit is hygienic: that it builds successfully on its own without warnings, errors, regressions, or test failures.
57+- Use `void *ptr` instead of `void* ptr`.
50+- Local variables containing secret data should be cleared explicitly to try to delete secrets from memory.
51+- Use `secp256k1_memcmp_var` instead of `memcmp` (see [#823](https://github.com/bitcoin-core/secp256k1/issues/823)).
52+- Arguments of the publicly-facing API must have a specific order defined in [include/secp256k1.h](include/secp256k1.h).
53+
54+#### Style Conventions
55+
_secp256k1.
38+
39+## Contributor Workflow & Peer Review
40+
41+The Contributor Workflow & Peer Review in libsecp256k1 are similar to Bitcoin Core's workflow and review processes described in its [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md).
42+
43+### Coding Conventions
A larger addition that comes to my mind is some explanation for ARG_CHECK, VERIFY, VERIFY_CHECK.
(Could also be done later in another PR. I think the most important thing here to get started with a document…)
47+- No runtime heap allocation (e.g., no `malloc`).
48+- The tests should cover all lines and branches of the library (see [Test coverage](#coverage)).
49+- Operations involving secret data should be tested for being constant time with respect to the secrets (see [src/ctime_tests.c](src/ctime_tests.c)).
50+- Local variables containing secret data should be cleared explicitly to try to delete secrets from memory.
51+- Use `secp256k1_memcmp_var` instead of `memcmp` (see [#823](https://github.com/bitcoin-core/secp256k1/issues/823)).
52+- Arguments of the publicly-facing API must have a specific order defined in [include/secp256k1.h](include/secp256k1.h).
nit: should this point go to style conventions?
Alternatively, and depending on your definition, some of the points I suggested for style conventions should go here… I don’t care too much.
81+ $ gcovr --exclude 'src/bench*' --html --html-details -o coverage/coverage.html
82+
83+#### Exhaustive Tests
84+
85+There are tests of several functions in which a small group replaces secp256k1.
86+These tests are *exhaustive* since they provide all elements and scalars of the small group as input arguments (see [src/tests_exhaustive.c]).
0These tests are *exhaustive* since they provide all elements and scalars of the small group as input arguments (see [src/tests_exhaustive.c](src/tests_exhaustive.c)).
53+
54+#### Style conventions
55+
56+- Commits should be atomic and diffs should be easy to read. For this reason, do not mix any formatting fixes or code moves with actual code changes. Make sure each individual commit is hygienic: that it builds successfully on its own without warnings, errors, regressions, or test failures.
57+- New code should adhere to the style of existing, in particular surrounding, code. Other than that, we do not enforce strict rules for code formatting.
58+- Use `void *ptr` instead of `void* ptr`.
We forgot the most important thing…
0- The code conforms to C89. Most notably, that means that only `/* ... */` comments are allowed (no `//` line comments). Moreover, any declarations in a `{ ... }` block (e.g., a function) must appear at the beginning of the block before any statements. When you would like to declare a variable in the middle of a block, you can open a new block:
1 ```C
2 void secp256k_foo(void) {
3 unsigned int x; /* declaration */
4 int y = 2*x; /* declaration */
5
6 x = 17; /* statement */
7 {
8 int a, b; /* declaration */
9
10 a = x + y; /* statement */
11 secp256k_bar(x, &b); /* statement */
12 }
13 }
14 ```
15- Use `unsigned int` instead of just `int`.
16- Use `void *ptr` instead of `void* ptr`.
(Note the indentation with 4 spaces to have the ``` block in the bullet point, I had to look this up…)
int for booleans.
Ah, lol, I meant to write this:
“Use unsigned int instead of just unsigned”.
So this was just a remark about spelling out the entire type.
But yeah, we should add a bullet point about integers, but perhaps in the next PR. It’s a bit more complicated with the stdint.h types etc.
libsecp256k1 has become more than a library for just ECDSA and key tweaking.
Additional points suggested in this thread: