Context isn’t freed in the ECDH benchmark #722

1. 
   elichai commented at 2:40 pm on March 4, 2020: contributor

   Hi, Currently the context is leaked in bench_ecdh. Wanted to prevent this happening in the future by adding it to the valgrind run in travis. for that I made run_benchmark pass the number of iterations as a variable into the benchmarks so that the iteration number it uses to print results actually matches the one used in the benchmark. and added a way to pass -DITERS=N to change the baseline number of iterations used in the benchmarks(so that we can run the benchmarks with ITERS=1 under travis to make sure they compile, run, don’t leak or have UB)
2. free the ctx at the end of bench_ecdh 02dd5f1bbb
3. elichai force-pushed on Mar 4, 2020
4. 
   real-or-random commented at 3:21 pm on March 4, 2020: contributor

   Concept ACK.

   This is neat. Travis fails at the moment: https://travis-ci.org/bitcoin-core/secp256k1/jobs/658249366#L447

   A few observations:

   • If we already make iter configurable, wouldn’t it be more convenient to obtain the parameter at runtime?
   • Should we make count configurable too?
5. 
   elichai commented at 3:27 pm on March 4, 2020: contributor

   * If we already make `iter` configurable, wouldn't it be more convenient to obtain the parameter at runtime?
   

   That’s what I did at first, but it somewhat collides with have_flag so I decided to start with a simpler version and if people think it’s a useful feature I can implement have_flag in a way that will work both with the number of iterations and the flags without ambiguity(ie --tests=N)

   * Should we make `count` configurable too?
   

   Count doesn’t seem to affect performance that match, so I didn’t need to do that. but if we’ll add runtime configurations we can definitely add that too.

6. 
   elichai commented at 3:31 pm on March 4, 2020: contributor

   Concept ACK.

   This is neat. Travis fails at the moment: https://travis-ci.org/bitcoin-core/secp256k1/jobs/658249366#L447

   Right, forgot I need to check if recovery and/or ecdh are compiled or not. that will be a big enough logic that I’m thinking of moving this to autotools maybe (although that will make it hard again to run under valgrind) Will fix this later/tomorrow and see how complex it will look

7. elichai force-pushed on Mar 4, 2020
8. elichai force-pushed on Mar 4, 2020
9. 
   sipa commented at 3:16 am on March 5, 2020: contributor

   configure: error: unrecognized option: `-DITERS=1'
10. elichai force-pushed on Mar 5, 2020
11. 
    jonasnick commented at 9:06 pm on March 9, 2020: contributor

    I agree that it would be better to make the benchmark iterations runtime configurable. Perhaps getenv would be an acceptable workaround?
12. 
    real-or-random commented at 7:37 am on March 10, 2020: contributor

    I agree that it would be better to make the benchmark iterations runtime configurable. Perhaps getenv would be an acceptable workaround?

    I think an environment variable is not bad because it can persist and it applies to all benchmarks. That fits our usage of the benchmarks.

13. elichai force-pushed on Mar 11, 2020
14. in src/bench.h:125 in 98812c5d2f outdated

    120@@ -121,4 +121,13 @@ int have_flag(int argc, char** argv, char *flag) {
    121     return 0;
    122 }
    123 
    124+int get_iters(int default_iters) {
    125+    char* env = getenv("ITERS");
    

    ---

    ---

    

    real-or-random commented at 11:00 am on March 11, 2020:

    maybe change it to something with more meaning and less likely to collide with other software, e.g., SECP256K1_BENCH_ITERS

    ---

    

    elichai commented at 11:01 am on March 11, 2020:

    even though it’s just the for the benchmarks? we also have int have_flag(..); and void print_number(..); in that header.

    ---

    

    real-or-random commented at 11:06 am on March 11, 2020:

    I’m talking about the environment variable, not the function. :)

    ---

    

    elichai commented at 11:13 am on March 11, 2020:

    Done
15. real-or-random approved
16. 
    real-or-random commented at 11:00 am on March 11, 2020: contributor

    ACK except nit
17. elichai force-pushed on Mar 11, 2020
18. real-or-random approved
19. 
    real-or-random commented at 11:50 am on March 11, 2020: contributor

    ACK https://github.com/bitcoin-core/secp256k1/pull/722/commits/31c3d6ccda432b35d1d83b72bcb9ba6f3c0a3166 I’ve looked at the diff but I haven’t tested the changes
20. in .travis.yml:97 in 31c3d6ccda outdated

    93@@ -94,6 +94,12 @@ script:
    94    travis_wait 30 valgrind --error-exitcode=42 ./tests 16 &&
    95    travis_wait 30 valgrind --error-exitcode=42 ./exhaustive_tests;
    96    fi
    97+ - if [ -n "$BENCH" ]; then
    

    ---

    ---

    

    jonasnick commented at 8:26 pm on March 11, 2020:

    BENCH is never set

    ---

    

    elichai commented at 8:41 pm on March 11, 2020:

    oh I broke it when I replaced the precompiled macro with the env. Thanks!

    ---

    

    elichai commented at 9:49 am on March 13, 2020:

    Done

    ---

    

    jonasnick commented at 6:45 pm on March 16, 2020:

    Better to unset BENCH int the BUILD=distcheck case similar to CTIMETEST because the bench binaries are not build in that case.

    ---

    

    elichai commented at 2:03 pm on March 18, 2020:

    Hmm so why didn’t the tests fail

    ---

    

    elichai commented at 2:05 pm on March 18, 2020:

    Maybe because there’s ; between the ifs? hmmm

    ---

    

    elichai commented at 2:16 pm on March 18, 2020:

    Got it to fail: https://travis-ci.org/github/elichai/secp256k1/jobs/663963345 Will fix now. Thanks.

    ---

    

    real-or-random commented at 2:18 pm on March 18, 2020:

    ugh yep, I think we want && to make sure that the final exit code of the statement.

    I think it’s cleaner to have individual lines in yaml even.

    ---

    

    real-or-random commented at 2:20 pm on March 18, 2020:

    I think it’s cleaner to have individual lines in yaml even.

    Then the execution continues. With && we stop after the first failing command, which is okay but not optimal.

    ---

    

    elichai commented at 10:43 am on March 19, 2020:

    Can you split an if across different yaml lines?

    ---

    

    real-or-random commented at 12:33 pm on March 19, 2020:

    Now, but you could different ifs with combined conditions on different lines. If you want to try, feel free to add a commit on top. Otherwise tell us and let’s get his merged, it’s certainly good as it is.

    ---

    

    elichai commented at 12:47 pm on March 19, 2020:

    I’m kinda tired of fighting with travis, so unless there’s any objections I prefer to get it in as is
21. in src/bench_internal.c:375 in 3f5e420c27 outdated

    394+    if (have_flag(argc, argv, "hash") || have_flag(argc, argv, "sha256")) run_benchmark("hash_sha256", bench_sha256, bench_setup, NULL, &data, 10, iters);
    395+    if (have_flag(argc, argv, "hash") || have_flag(argc, argv, "hmac")) run_benchmark("hash_hmac_sha256", bench_hmac_sha256, bench_setup, NULL, &data, 10, iters);
    396+    if (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);
    397 
    398     if (have_flag(argc, argv, "context") || have_flag(argc, argv, "verify")) run_benchmark("context_verify", bench_context_verify, bench_setup, NULL, &data, 10, 20);
    399     if (have_flag(argc, argv, "context") || have_flag(argc, argv, "sign")) run_benchmark("context_sign", bench_context_sign, bench_setup, NULL, &data, 10, 200);
    

    ---

    ---

    

    jonasnick commented at 8:29 pm on March 11, 2020:

    Is this and context_verify intentionally left with 200 iters? Would be more consistent to use 1 + iters/100 or something.

    ---

    

    elichai commented at 8:42 pm on March 11, 2020:

    nope. missed it

    ---

    

    elichai commented at 9:49 am on March 13, 2020:

    Done
22. in src/bench_ecmult.c:192 in 3f5e420c27 outdated

    191     }
    192 
    193-    for (p = 0; p <= 11; ++p) {
    194-        for (i = 9; i <= 16; ++i) {
    195-            run_test(&data, i << p, 1);
    196+    if (iters > 1) {
    

    ---

    ---

    

    jonasnick commented at 8:30 pm on March 11, 2020:

    Would be good to add comment why that’s useful.

    ---

    

    elichai commented at 9:49 am on March 13, 2020:

    Done
23. in src/bench_sign.c:17 in 3f5e420c27 outdated

    13@@ -14,6 +14,7 @@ typedef struct {
    14     unsigned char key[32];
    15 } bench_sign;
    16 
    17+
    

    ---

    ---

    

    jonasnick commented at 8:31 pm on March 11, 2020:

    nit: There are 2 more unnecessary newlines I think.

    ---

    

    elichai commented at 9:49 am on March 13, 2020:

    Done
24. 
    gmaxwell commented at 10:40 pm on March 11, 2020: contributor

    FWIW, when you setup ‘fast’ CI tests, please at least run them with 2 iterations, not 1. 1 iteration will fail to catch some kinds of bugs in the test harnesses, e.g. use after free if the deallocations are in the wrong place with the testing loops, etc. If 2 is too much, maybe resize the underlying work units so that they’re not.
25. Pass num of iters to benchmarks as variable, and define envvar ca4906b02e
26. elichai force-pushed on Mar 13, 2020
27. 
    jonasnick commented at 6:45 pm on March 16, 2020: contributor

    ACK mod nit
28. Add running benchmarks regularly and under valgrind in travis 85b35afa76
29. elichai force-pushed on Mar 18, 2020
30. jonasnick approved
31. 
    jonasnick commented at 10:20 pm on March 18, 2020: contributor

    ACK 85b35afa765ba23ac3682cf15800769b0a3b834d
32. jonasnick cross-referenced this on Mar 24, 2020 from issue Consider removing x86_64 field assembly by jonasnick
33. 
    real-or-random commented at 2:35 pm on March 24, 2020: contributor

    ACK 85b35afa765ba23ac3682cf15800769b0a3b834d I looked at the diff and tested the ecdh benchmark using valgrind
34. jonasnick merged this on Mar 24, 2020
35. jonasnick closed this on Mar 24, 2020

    ---

36. 
    real-or-random commented at 4:08 pm on March 24, 2020: contributor

    This is minor inconvenience that I can fix on the way, e.g. in #723:

    0> SECP256K1_BENCH_ITERS= ./bench_ecdh 
    1[1]    96654 floating point exception (core dumped)  
    

37. deadalnix referenced this in commit 270f3194d8 on Jun 4, 2020
38. deadalnix referenced this in commit a29a90ca04 on Jun 5, 2020
39. sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa
40. fanquake referenced this in commit 8c97780db8 on Jun 13, 2020
41. sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020
42. ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020
43. UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021
44. 5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021
45. gades referenced this in commit d855cc511d on May 8, 2022

Contributors
elichai real-or-random sipa jonasnick gmaxwell