ctime_test: move context randomization test to the end #894

pull jonasnick wants to merge 1 commits into bitcoin-core:master from jonasnick:fix-ctime changing 1 files +36 −26

jonasnick commented at 11:25 PM on February 4, 2021: contributor

I noticed this while reviewing https://github.com/ElementsProject/secp256k1-zkp/pull/117 and finding some seemingly unnecessary VALGRIND_MAKE_MEM_DEFINED that I couldn't remove until I saw the bug.
gmaxwell commented at 4:35 AM on February 5, 2021: contributor

ah, indeed the comment said it needed to be last. :) Did you look into why it wasn't causing failures with the schnorrsig test?
in src/valgrind_ctime_test.c:60 in 6245eec03a outdated
```
  55 | +    ret = secp256k1_context_randomize(ctx, key);
  56 | +    VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
  57 | +    CHECK(ret);
  58 | +
  59 | +    secp256k1_context_destroy(ctx);
  60 | +    return 1;
```
real-or-random commented at 9:04 AM on February 5, 2021:

This must be left 0. This is main. The issue here is valgrind exits with the exit code of the underlying process (unless there's a valgrind error, then we tell it to return 42). Successful runs will return 1 and that's why CI fails.

jonasnick commented at 9:15 AM on February 5, 2021:

oops yeah that was unintentional
real-or-random approved
real-or-random commented at 9:05 AM on February 5, 2021: contributor

Concept ACK

nice catch and good idea to extract a function

edit: Ah, I didn't want to github-"approve". Anyway, we don't give attention to this.
jonasnick force-pushed on Feb 5, 2021
jonasnick commented at 10:50 AM on February 5, 2021: contributor

Did you look into why it wasn't causing failures with the schnorrsig test?

The reason is that in ctime_test.c we declassify public keys after creating them. In the case of the schnorrsig & extrakeys tests, they only operate on keypairs, and the public part of a keypair is declassified on keypair_load because it needs to branch on it.
ctime_test: move context randomization test to the end 7d3497cdc4
in src/valgrind_ctime_test.c:38 in 202a4af813 outdated
```
  33 | +    int ret, i;
  34 | +
  35 | +    if (!RUNNING_ON_VALGRIND) {
  36 | +        fprintf(stderr, "This test can only usefully be run inside valgrind.\n");
  37 | +        fprintf(stderr, "Usage: libtool --mode=execute valgrind ./valgrind_ctime_test\n");
  38 | +        exit(1);
```
real-or-random commented at 11:15 AM on February 5, 2021:

Since you're touching this anyway, can you #include <stdlib.h> (or simply return 1 here)?

real-or-random commented at 11:17 AM on February 5, 2021:

Oh and we also need stdio for fprintf.

jonasnick commented at 2:38 PM on February 5, 2021:

done
jonasnick force-pushed on Feb 5, 2021
real-or-random approved
real-or-random commented at 3:25 PM on February 5, 2021: contributor

ACK 7d3497cdc4c747bdd51db70f42fe218622c3169f diff looks good
jonasnick cross-referenced this on Feb 5, 2021 from issue Add ECDSA adaptor signatures module by jesseposner
jonasnick merged this on Feb 22, 2021
jonasnick closed this on Feb 22, 2021
deadalnix referenced this in commit 5c69fab896 on Feb 23, 2021
jesseposner referenced this in commit 54985c2b87 on Mar 5, 2021
Fabcien referenced this in commit 9ea4bda3ab on Apr 8, 2021
deadalnix referenced this in commit 63326005a7 on Apr 9, 2021

Contributors

jonasnick

gmaxwell

real-or-random