tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID #1390

pull jonasnick wants to merge 4 commits into bitcoin-core:master from jonasnick:check-illegal changing 5 files +180 −501

jonasnick commented at 10:04 PM on July 28, 2023: contributor

Fixes #1167
jonasnick force-pushed on Jul 29, 2023

in src/tests.c:6631 in 7120fc25a3 outdated

6620 | +        {
6621 | +            int32_t ecount = 0;
6622 | +            secp256k1_context_set_illegal_callback(CTX, counting_illegal_callback_fn, &ecount);
6623 | +            CHECK(secp256k1_ec_pubkey_cmp(CTX, &pk_tmp, &pk_tmp) == 0);
6624 | +            CHECK(ecount == 2);
6625 | +            secp256k1_context_set_illegal_callback(CTX, NULL, NULL);

real-or-random commented at 12:01 PM on July 31, 2023:

Is it a bug that secp256k1_ec_pubkey_cmp calls the callback twice?

I mean, we never do this anywhere else, even if multiple arguments are illegal. In particular, ARG_CHECK makes the function return immediately on failure.

jonasnick commented at 12:53 PM on July 31, 2023:

It's intentional, see the comment in pubkey_cmp:

        /* If the public key is NULL or invalid, ec_pubkey_serialize will call
         * the illegal_callback and return 0. In that case we will serialize the
         * key as all zeros which is less than any valid public key. This
         * results in consistent comparisons even if NULL or invalid pubkeys are
         * involved and prevents edge cases such as sorting algorithms that use
         * this function and do not terminate as a result. */

In particular, returning 0 immediately in pubkey_cmp if parsing fails does not signal an error to the color, but that both public keys are identical.

real-or-random commented at 1:57 PM on July 31, 2023:

Oh, crazy. I still think we could get the best of both worlds and retain the current all-zeros behavior and at the same time call the callback only once. But that's better done in a separate PR then.

in src/modules/extrakeys/tests_impl.h:138 in 7120fc25a3 outdated

 147 | +    {
 148 | +        int32_t ecount = 0;
 149 | +        secp256k1_context_set_illegal_callback(CTX, counting_illegal_callback_fn, &ecount);
 150 | +        CHECK(secp256k1_xonly_pubkey_cmp(CTX, &pk1, &pk1) == 0);
 151 | +        CHECK(ecount == 2);
 152 | +        secp256k1_context_set_illegal_callback(CTX, NULL, NULL);

real-or-random commented at 12:02 PM on July 31, 2023:

(if you change it, also change it here)

in src/modules/ecdh/tests_impl.h:28 in 7120fc25a3 outdated

  25 | @@ -26,31 +26,19 @@ static int ecdh_hash_function_custom(unsigned char *output, const unsigned char
  26 |  
  27 |  static void test_ecdh_api(void) {
  28 |      /* Setup context that just counts errors */

real-or-random commented at 12:07 PM on July 31, 2023:

nit: Comment needs to be removed (or changed)

jonasnick commented at 1:20 PM on July 31, 2023:

fixed

in src/tests.c:5939 in 7120fc25a3 outdated
```
5915 | @@ -5962,142 +5916,99 @@ static void run_ec_pubkey_parse_test(void) {
5916 |          0xB8, 0x00
5917 |      };
5918 |      unsigned char sout[65];
5919 | -    unsigned char shortkey[2];
5920 | +    unsigned char shortkey[2] = { 0 };
```
real-or-random commented at 12:19 PM on July 31, 2023:

nit:

unsigned char shortkey[2] = { 0x03, 0x05 };

0x0500000000000000000000000000000000000000000000000000000000000000 is a valid x-coord with odd y-coord
jonasnick commented at 12:43 PM on July 31, 2023:

I'm not sure how this would improves the tests.

real-or-random commented at 1:52 PM on July 31, 2023:

Yeah sorry, I didn't think it through entirely.
real-or-random commented at 12:20 PM on July 31, 2023: contributor
utACK mod nits

This makes the test code much cleaner.

I suggest (this PR or followup)
- changing secp256k1_ec_pubkey_cmp to call a callback at most once (see review)
- adding a macro CHECK_ERROR for the error_callback (which should probably involve renaming counting_illegal_callback to just counting_callback -- the callback doesn't care what it's used for)
real-or-random added the label assurance on Jul 31, 2023
real-or-random added the label refactor/smell on Jul 31, 2023
jonasnick force-pushed on Jul 31, 2023
jonasnick commented at 1:20 PM on July 31, 2023: contributor
adding a macro CHECK_ERROR for the error_callback (which should probably involve renaming counting_illegal_callback to just counting_callback -- the callback doesn't care what it's used for)
done
real-or-random commented at 2:29 PM on July 31, 2023: contributor
Ah, my intention behind the non-VOID macros is that they can also be used for functions that return pointers (because NULL == 0 as guaranteed by the standard), e.g., as in the existing line:
```
CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_preallocated_clone(STATIC_CTX, my_static_ctx));
```
That means we could also have CHECK_ERROR and use it. Do these fixups make sense? https://github.com/real-or-random/secp256k1/commits/check-illegal I also tried to clarify the comments to say that NULL pointers are covered.

edit: Alternatively, if you think that == NULL is more explicit, we should probably do this everywhere.
real-or-random commented at 2:37 PM on July 31, 2023: contributor

The set_illegal_callback calls in ec_pubkey_parse_pointtest and run_eckey_edge_case_test should also be removed.
jonasnick force-pushed on Jul 31, 2023
real-or-random approved
real-or-random commented at 9:21 AM on August 1, 2023: contributor

utACK fb5eb75a89598394f615eb55f0757e7c35e420ba
siv2r commented at 4:41 AM on August 29, 2023: contributor

The error_callback and illegal_callback are being set to NULL at the end of test_keypair fn (in extrakeys/tests_impl.h). Shouldn't we remove these too?

Github doesn't allow me to comment on these lines directly. So, here's the link: https://github.com/bitcoin-core/secp256k1/pull/1390/files#diff-07d311190e50e344466bc33ed56c60d2a15f4e8221bfc97792e1cd441b8f3e19L443-L444
siv2r commented at 4:44 AM on August 29, 2023: contributor

ACK fb5eb75, the tests locally pass on my machine (for all four commits).
real-or-random commented at 9:48 AM on August 29, 2023: contributor

needs rebase :)
tests: remove unnecessary set_illegal_callback 875b0ada25

tests: remove unnecessary test in run_ec_pubkey_parse_test

This test tested whether setting the callback works correctly which should be
tested in the context tests.

a1d52e3e12

jonasnick force-pushed on Sep 4, 2023
jonasnick commented at 12:53 PM on September 4, 2023: contributor

rebased

tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID

This commit also explicitly initializes shortpubkey. For some reason, removing
surrounding, unrelated lines results in gcc warnings when configured with
--enable-ctime-tests=no --with-valgrind=no.

f8d7ea68df

tests: add CHECK_ERROR_VOID and use it in scratch tests 70303643cf
jonasnick force-pushed on Sep 4, 2023
jonasnick commented at 4:20 PM on September 4, 2023: contributor

@siv2r

The error_callback and illegal_callback are being set to NULL at the end of test_keypair fn (in extrakeys/tests_impl.h).

Good catch, fixed.
real-or-random approved
real-or-random commented at 4:27 PM on September 4, 2023: contributor

reACK 70303643cf42d18acbf1c020480c6bb23072dbd9
siv2r commented at 4:55 PM on September 4, 2023: contributor

reACK 7030364 (tests pass locally)
real-or-random merged this on Sep 4, 2023
real-or-random closed this on Sep 4, 2023
real-or-random referenced this in commit d575ef9aca on Oct 12, 2023
fanquake referenced this in commit 41e1b677ca on Jan 3, 2024
fanquake referenced this in commit 29fde0223a on Jan 4, 2024
janus referenced this in commit 5fe435c9b2 on Apr 6, 2024
hebasto referenced this in commit b6de625950 on May 11, 2024
delta1 referenced this in commit 6089844b3c on Apr 2, 2025
div72 referenced this in commit af627d47c3 on Apr 12, 2025
str4d referenced this in commit 136aa9fe62 on Jun 4, 2025
oskarszoon referenced this in commit 0d5c9260f4 on Jul 1, 2025
real-or-random referenced this in commit baa265429f on Sep 24, 2025

Contributors

jonasnick

real-or-random

siv2r

Labels

assurance tweak/refactor

Linked (view graph)

#1167 Setting counting_illegal_callback may hide failing tests #1727 docs: Clarify that callback can be called more than once