Instead of just testing properties of the points xG for x=-36..36:
3722 void test_ecmult_constants(void) {
3723- /* Test ecmult_gen() for [0..36) and [order-36..0). */
3724+ /* Test ecmult_gen for:
3725+ * - Numbers 0..36 and their negations
3726+ * - Numbers 2^i (with i=0..255)
3727+ * - Numbers 2^i + 2^j (with i=0..255, j=i+1..255)
i only goes up to 254.
a*(2^N)^b a=1..M b=0..256/M, for some M and N, as that’d give better coverage of the actual tables involved.
3689@@ -3690,37 +3690,77 @@ void run_wnaf(void) {
3690 CHECK(secp256k1_scalar_is_zero(&n));
3691 }
3692
3693+void test_ecmult_accumulate(secp256k1_sha256* acc, const secp256k1_scalar* x) {
3694+ /* Compute x*G in 4 different ways, serialize it uncompressed, and feed it into acc. */
secp256k1_ecmult_multi_var is out of scope as a 5th way for this test?
ACK ec6dff0f532b9f029de6c64af1cd0480651f51f2
I don’t have enough context to comment on the choice of test cases (e.g. whether there are other precomputed values that would be good to test), but the test itself looks good and passes locally.
I also checked against an independent Python implementation and got the exact same SHA256 output.
FWIW, the Python code to recreate the hash (in the https://github.com/bitcoin/bitcoin/tree/22.0/test/function directory):
0from test_framework import key
1import hashlib
2
3s = hashlib.sha256()
4
5def add(s, v):
6 v = v % key.SECP256K1_ORDER
7 if v == 0:
8 s.update(b"\x00")
9 else:
10 keyb = v.to_bytes(32, 'big')
11 ec = key.ECKey()
12 ec.set(keyb, False)
13 s.update(ec.get_pubkey().get_bytes())
14
15for i in range(0, 37):
16 add(s, i)
17 add(s, -i)
18
19for b in range(256):
20 for v in range(1, 256, 2):
21 add(s, v << b)
22
23print(s.hexdigest())
ACK 8ed167b4949898a531a57296b15f749037cc73d0
./configure --with-valgrind=yes; make; make check passed locallyEdit: My mistake, running make check locally isn’t the way to reproduce what happened in CI
TEST_ITERS for the valgrind build?
@sipa You rebased on the wrong branch.
https://github.com/sipa/secp256k1/tree/202104_ecmult_bitcomb says “This branch is 1 commit ahead, 158 commits behind bitcoin-core:master.”
4559- }
4560- for (i = 1; i <= 36; i++ ) {
4561+ secp256k1_sha256 acc;
4562+ unsigned char b32[32];
4563+ int i, j;
4564+ secp256k1_scratch_space *scratch = secp256k1_scratch_space_create(ctx, 65536);