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

pull jonasnick wants to merge 6 commits into bitcoin-core:master from jonasnick:202207-selftest-api-jn changing 15 files +312 −433
  1. jonasnick commented at 10:27 pm on December 1, 2022: contributor
    Based on #1126.
  2. 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.
  3. real-or-random cross-referenced this on Dec 2, 2022 from issue API cleanup with respect to contexts by real-or-random
  4. jonasnick force-pushed on Dec 5, 2022
  5. 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
  6. jonasnick force-pushed on Dec 5, 2022
  7. sipa commented at 7:07 pm on December 5, 2022: contributor
    Concept ACK.
  8. jonasnick force-pushed on Dec 6, 2022
  9. 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
  10. 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.
  11. 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.
  12. real-or-random commented at 4:22 pm on December 6, 2022: contributor
    Concept ACK
  13. jonasnick force-pushed on Dec 6, 2022
  14. 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
  15. 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
  16. real-or-random commented at 11:22 pm on December 6, 2022: contributor
    ACK mod nits
  17. benchmarks: Switch to NONE contexts 8d7a9a8eda
  18. tests: Switch to NONE contexts in exhaustive and ctime tests 37ba744f5b
  19. group: add gej_eq_var caa0ad631e
  20. tests: add test for deprecated flags and rm them from run_context 86540e9e1f
  21. tests: Switch to NONE contexts in tests.c 0c8a5caddd
  22. tests: Switch to NONE contexts in module tests d6dc0f4ae3
  23. 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.
  24. jonasnick force-pushed on Dec 7, 2022
  25. 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.

  26. real-or-random approved
  27. real-or-random commented at 11:52 am on December 7, 2022: contributor
    ACK d6dc0f4ae33d3cd25e9731b9d63b4a34600bc535 diff looks good and tests pass locally
  28. 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
  29. sipa commented at 2:55 pm on December 7, 2022: contributor
    utACK d6dc0f4ae33d3cd25e9731b9d63b4a34600bc535
  30. real-or-random merged this on Dec 7, 2022
  31. real-or-random closed this on Dec 7, 2022

  32. sipa referenced this in commit 9d47e7b71b on Dec 13, 2022
  33. dhruv referenced this in commit 55ffd47cc6 on Dec 14, 2022
  34. dhruv referenced this in commit 967c65b158 on Dec 14, 2022
  35. dhruv referenced this in commit 78b5ddf28b on Jan 11, 2023
  36. dhruv referenced this in commit 215394a1d5 on Jan 11, 2023
  37. div72 referenced this in commit 945b094575 on Mar 14, 2023
  38. str4d referenced this in commit 0df7b459f6 on Apr 21, 2023
  39. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  40. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  41. jonasnick cross-referenced this on Jul 18, 2023 from issue Upstream PRs 993, 1152, 1165, 1126, 1168, 1173, 1055 by jonasnick

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me