Replace deprecated context flags with NONE in benchmarks and tests

jonasnick commented at 10:27 pm on December 1, 2022: contributor

Based on #1126.

in src/tests.c:164 in 6112a72b21 outdated

159+    CHECK(SECP256K1_CONTEXT_DEFAULT == SECP256K1_CONTEXT_SIGN);
160+
161+    /* Check that a context created with any of the flags in the flags array is
162+     * identical to the DEFAULT context. We use the preallocated interface to
163+     * allow setting the memory area before creating the context and therefore
164+     * to ignore the padding when comparing the contexts. */

real-or-random commented at 8:31 am on December 2, 2022:

C doesn’t guarantee that the padding bytes will remain the same. In fact, the padding bytes have “indeterminate” value, i.e., you are allowed to read them but nothing is guaranteed about the value you get. Some even claim you can get different values in two consecutive reads, even without a write in between the two reads.

If we want to test this, then the proper way is to compare all members… This introduces an annoying coupling between the code and the tests: if we add a new member to the struct, the tests will stay silent but compare only the old members. I think the cleanest way would be ~~to ditch C and rewrite the library in Rust~~ add a “test-only” comparison function, either in secp256k1.c close to the definition of the struct (or alternatively in the tests, but then add a comment to the struct def “whenever you change this, change also context_compare in tests.c – not beautiful but does the job…) And it’s even worse because the context contains more structs, so we need to do that recursively. To be honest, that’s why I was too lazy to include a test of this kind in my PR…

jonasnick commented at 1:11 pm on December 5, 2022:

Thanks…. Added a function that compares the members.

real-or-random cross-referenced this on Dec 2, 2022 from issue API cleanup with respect to contexts by real-or-random

jonasnick force-pushed on Dec 5, 2022

real-or-random renamed this:
~~Replace deprecated context flags with DEFAULT in benchmarks and tests~~
Replace deprecated context flags with NONE in benchmarks and tests
on Dec 5, 2022

jonasnick force-pushed on Dec 5, 2022

sipa commented at 7:07 pm on December 5, 2022: contributor

Concept ACK.

jonasnick force-pushed on Dec 6, 2022

in src/tests.c:3885 in 5a752e5ffd outdated

3880+        random_gej_test(&b);
3881+        CHECK(!secp256k1_gej_eq_var(&a, &b));
3882+
3883+        b = a;
3884+        random_field_element_test(&fe);
3885+        if(secp256k1_fe_is_zero(&fe)) {

real-or-random commented at 2:28 pm on December 6, 2022:

micronit: add space after if

jonasnick commented at 9:59 pm on December 6, 2022:

fixed

in src/tests.c:165 in 7ac75e86a2 outdated

165+        return 0;
166+    }
167+    return 1;
168+}
169+
170+void test_deprecated_flags(void) {

real-or-random commented at 4:20 pm on December 6, 2022:

This could also test consistency of secp256k1_context_preallocated_clone_size

jonasnick commented at 9:59 pm on December 6, 2022:

I’m not sure how it relates to deprecated flags. If the contexts are all equal than the size of their clones is equal.

real-or-random commented at 11:09 pm on December 6, 2022:

You’re right.

in src/tests.c:155 in 7ac75e86a2 outdated

150+    if (a->built != b->built
151+        || !secp256k1_scalar_eq(&a->blind, &b->blind)
152+        || !secp256k1_gej_eq_var(&a->initial, &b->initial)) {
153+        return 0;
154+    }
155+    return 1;

real-or-random commented at 4:21 pm on December 6, 2022:

nit: maybe just return ( ... && ... && ...) ? (same below)

jonasnick commented at 10:00 pm on December 6, 2022:

An AI wrote this code for me. I did not review it. Fixed now.

real-or-random commented at 4:22 pm on December 6, 2022: contributor

Concept ACK

jonasnick force-pushed on Dec 6, 2022

in src/bench_internal.c:390 in 5d9c466712 outdated

386@@ -395,8 +387,7 @@ int main(int argc, char **argv) {
387     if (d || have_flag(argc, argv, "hash") || have_flag(argc, argv, "hmac")) run_benchmark("hash_hmac_sha256", bench_hmac_sha256, bench_setup, NULL, &data, 10, iters);
388     if (d || have_flag(argc, argv, "hash") || have_flag(argc, argv, "rng6979")) run_benchmark("hash_rfc6979_hmac_sha256", bench_rfc6979_hmac_sha256, bench_setup, NULL, &data, 10, iters);
389 
390-    if (d || have_flag(argc, argv, "context") || have_flag(argc, argv, "verify")) run_benchmark("context_verify", bench_context_verify, bench_setup, NULL, &data, 10, 1 + iters/1000);
391-    if (d || have_flag(argc, argv, "context") || have_flag(argc, argv, "sign")) run_benchmark("context_sign", bench_context_sign, bench_setup, NULL, &data, 10, 1 + iters/100);
392+    if (d || have_flag(argc, argv, "context")) run_benchmark("context_create", bench_context, bench_setup, NULL, &data, 10, 1 + iters/1000);

real-or-random commented at 11:16 pm on December 6, 2022:

nit: I think iters is better than iters/1000 now that context creation is very fast

jonasnick commented at 10:56 am on December 7, 2022:

makes sense, done

in src/tests.c:316 in 8a8d60c287 outdated

317-    CHECK(secp256k1_ecdsa_sig_sign(&both->ecmult_gen_ctx, &sigr, &sigs, &key, &msg, &nonce, NULL));
318+    CHECK(secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &sigr, &sigs, &key, &msg, &nonce, NULL));
319 
320     /* try verifying */
321     CHECK(secp256k1_ecdsa_sig_verify(&sigr, &sigs, &pub, &msg));
322     CHECK(secp256k1_ecdsa_sig_verify(&sigr, &sigs, &pub, &msg));

real-or-random commented at 11:21 pm on December 6, 2022:

now that you touch this, maybe remove the duplicate line

jonasnick commented at 10:56 am on December 7, 2022:

done

real-or-random commented at 11:22 pm on December 6, 2022: contributor

ACK mod nits

benchmarks: Switch to NONE contexts 8d7a9a8eda

tests: Switch to NONE contexts in exhaustive and ctime tests 37ba744f5b

group: add gej_eq_var caa0ad631e

tests: add test for deprecated flags and rm them from run_context 86540e9e1f

tests: Switch to NONE contexts in tests.c 0c8a5caddd

tests: Switch to NONE contexts in module tests d6dc0f4ae3

in src/secp256k1.c:61 in 8a8d60c287 outdated

56@@ -57,6 +57,8 @@
57     } \
58 } while(0)
59 
60+/* Note that whenever you change the context struct, you must also change the
61+ * context_eq function. */

real-or-random commented at 11:23 pm on December 6, 2022:

maybe add “in the tests”

jonasnick commented at 10:56 am on December 7, 2022:

My thinking was that if we ever move this then it’s unlikely we update this line. So just having someone searching it is more robust.

real-or-random commented at 11:37 am on December 7, 2022:

Agreed.

jonasnick force-pushed on Dec 7, 2022

in src/modules/extrakeys/tests_impl.h:249 in d6dc0f4ae3

244@@ -260,33 +245,32 @@ void test_xonly_pubkey_tweak_check(void) {
245     unsigned char tweak[32];
246 
247     int ecount;
248-    secp256k1_context *none = api_test_context(SECP256K1_CONTEXT_NONE, &ecount);
249-    secp256k1_context *sign = api_test_context(SECP256K1_CONTEXT_SIGN, &ecount);
250-    secp256k1_context *verify = api_test_context(SECP256K1_CONTEXT_VERIFY, &ecount);
251+
252+    set_counting_callbacks(ctx, &ecount);

real-or-random commented at 11:48 am on December 7, 2022:

Here’s an example of #1167 in the wild. (maybe that’s how you found this issue?)

Anyway, we have this pattern many times in the tests. This needs separate solution that should not hold off this PR.

real-or-random approved

real-or-random commented at 11:52 am on December 7, 2022: contributor

ACK d6dc0f4ae33d3cd25e9731b9d63b4a34600bc535 diff looks good and tests pass locally

in src/tests.c:160 in 86540e9e1f outdated

155+int context_eq(const secp256k1_context *a, const secp256k1_context *b) {
156+    return a->declassify == b->declassify
157+            && ecmult_gen_context_eq(&a->ecmult_gen_ctx, &b->ecmult_gen_ctx)
158+            && a->illegal_callback.fn == b->illegal_callback.fn
159+            && a->illegal_callback.data == b->illegal_callback.
160+data

sipa commented at 2:51 pm on December 7, 2022:

This data on a new line is a bit ugly.

real-or-random commented at 3:25 pm on December 7, 2022:

I’m about to open a follow up, let me fix it there

sipa commented at 2:55 pm on December 7, 2022: contributor

utACK d6dc0f4ae33d3cd25e9731b9d63b4a34600bc535

real-or-random merged this on Dec 7, 2022

real-or-random closed this on Dec 7, 2022

sipa referenced this in commit 9d47e7b71b on Dec 13, 2022

dhruv referenced this in commit 55ffd47cc6 on Dec 14, 2022

dhruv referenced this in commit 967c65b158 on Dec 14, 2022

dhruv referenced this in commit 78b5ddf28b on Jan 11, 2023

dhruv referenced this in commit 215394a1d5 on Jan 11, 2023

div72 referenced this in commit 945b094575 on Mar 14, 2023

str4d referenced this in commit 0df7b459f6 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 18, 2023 from issue Upstream PRs 993, 1152, 1165, 1126, 1168, 1173, 1055 by jonasnick

Replace deprecated context flags with NONE in benchmarks and tests #1168