Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make test count iteration configurable by environment variable #851

Merged
merged 2 commits into from Dec 1, 2020

Conversation

apoelstra
Copy link
Contributor

You can set the test iteration count by command-line parameter but it is very hard to control the iteration count used by make check and even harder to control the count when this is run through Bitcoin Core's build system. Using an environment var solves both problems.

@real-or-random
Copy link
Contributor

Nice, that's indeed useful.

ACK 0ce4554 I inspected the diff

Here are a few small annoying things that you can add to this PR if you want. If not, let's consider this a list for another PR:

  • Add check that aborts if not count > 0? Technically count == 0 is fine but I think noone wants to run this on purpose as it just disables some tests arbitrary.
  • I was tempted to say that we should have a ITERS also for the exhaustive tests but I think that's not true. The default of 2 makes a lot of sense for these, because the randomness is only used to randomize the representation in jacobian coordinates. But we should add the same count > 0 check there.
  • Document the environment variables. Since we don't have a .md for tests, the canonical place is in the "usage" output. (This is not optimal because the benchmarks have an env variable but no "usage" output. Yeah, ok.)

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0ce4554

@elichai
Copy link
Contributor

elichai commented Nov 23, 2020

utACK 0ce4554

  • Add check that aborts if not count > 0? Technically count == 0 is fine but I think noone wants to run this on purpose as it just disables some tests arbitrary.

I also wanted to do some sanitizing on the bench env var but hadn't gotten to doing it yet

secp256k1/src/bench.h

Lines 124 to 131 in ca4906b

int get_iters(int default_iters) {
char* env = getenv("SECP256K1_BENCH_ITERS");
if (env) {
return strtol(env, NULL, 0);
} else {
return default_iters;
}
}

@apoelstra
Copy link
Contributor Author

deadalnix was using count==0 :) #528

@elichai
Copy link
Contributor

elichai commented Nov 23, 2020

deadalnix was using count==0 :) #528

lol
I didn't fix yet because I think those env vars are for "advanced users only" and only tests/benchmarks so it doesn't matter too much

@apoelstra
Copy link
Contributor Author

I'm happy to add a count>0 check if people think it's valuable.

I don't think it's worth exposing a way to change the exhaustive test iteration count. I didn't even know there was one.

Unfortunately there is no usage text for the tests binary :). I could add an unconditional message above the "count =" line.

@sipa
Copy link
Contributor

sipa commented Nov 23, 2020

The count variable for the exhaustive tests changes the z coordinate for gej elements (which is randomized, not exhaustive).

I don't think there is much need to changing the default, though.

@real-or-random
Copy link
Contributor

deadalnix was using count==0 :) #528

yeah but even he says:

My concern here isn't really that 0 is a very useful thing to test for, but that it put the system is some weird state that reveal a bug either in libsecp256k1 or possibly in gcc's optimizer.

So I think we should just enforce > 0. But either way is fine.

I don't think there is much need to changing the default, though.

Indeed.

src/tests.c Outdated Show resolved Hide resolved
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a729a6c I inspected the diff but didn't test

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK mod nit

src/tests.c Outdated
}
}
if (count <= 0) {
fputs("An iteration count of 0 is not allowed.\n", stderr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: <= 0

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f4fa8d2

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f4fa8d2

@jonasnick jonasnick merged commit 8f0c6f1 into bitcoin-core:master Dec 1, 2020
@apoelstra apoelstra deleted the 2020-11--test-iters branch December 3, 2020 02:54
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 8, 2021
…iable

Summary:
```
You can set the test iteration count by command-line parameter but it is
very hard to control the iteration count used by make check and even
harder to control the count when this is run through Bitcoin Core's
build system. Using an environment var solves both problems.
```

Backport of [[bitcoin-core/secp256k1#851 | secp256k1#851]]

Test Plan:
  SECP256K1_TEST_ITERS=16 ninja check-secp256k1
Should succeed but:
  SECP256K1_TEST_ITERS=0 ninja check-secp256k1
Should fail with error `An iteration count of 0 or less is not allowed.`

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9375
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Apr 9, 2021
…iable

Summary:
```
You can set the test iteration count by command-line parameter but it is
very hard to control the iteration count used by make check and even
harder to control the count when this is run through Bitcoin Core's
build system. Using an environment var solves both problems.
```

Backport of [[bitcoin-core/secp256k1#851 | secp256k1#851]]

Test Plan:
  SECP256K1_TEST_ITERS=16 ninja check-secp256k1
Should succeed but:
  SECP256K1_TEST_ITERS=0 ninja check-secp256k1
Should fail with error `An iteration count of 0 or less is not allowed.`

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants