tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL

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:

0        /* If the public key is NULL or invalid, ec_pubkey_serialize will call
1         * the illegal_callback and return 0. In that case we will serialize the
2         * key as all zeros which is less than any valid public key. This
3         * results in consistent comparisons even if NULL or invalid pubkeys are
4         * involved and prevents edge cases such as sorting algorithms that use
5         * 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:

0    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:

0CHECK_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

tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID #1390