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: contributorFixes #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:fixedin 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: contributorutACK 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 theerror_callback
(which should probably involve renamingcounting_illegal_callback
to justcounting_callback
– the callback doesn’t care what it’s used for)
real-or-random added the label assurance on Jul 31, 2023real-or-random added the label refactor/smell on Jul 31, 2023jonasnick force-pushed on Jul 31, 2023jonasnick 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: contributorAh, 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: contributorTheset_illegal_callback
calls inec_pubkey_parse_pointtest
andrun_eckey_edge_case_test
should also be removed.jonasnick force-pushed on Jul 31, 2023real-or-random approvedreal-or-random commented at 9:21 am on August 1, 2023: contributorutACK fb5eb75a89598394f615eb55f0757e7c35e420basiv2r commented at 4:41 am on August 29, 2023: contributorThe
error_callback
andillegal_callback
are being set toNULL
at the end oftest_keypair
fn (inextrakeys/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: contributorACK fb5eb75, the tests locally pass on my machine (for all four commits).real-or-random commented at 9:48 am on August 29, 2023: contributorneeds rebase :)tests: remove unnecessary set_illegal_callback 875b0ada25tests: 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.
jonasnick force-pushed on Sep 4, 2023jonasnick commented at 12:53 pm on September 4, 2023: contributorrebasedtests: 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.
tests: add CHECK_ERROR_VOID and use it in scratch tests 70303643cfjonasnick force-pushed on Sep 4, 2023real-or-random approvedreal-or-random commented at 4:27 pm on September 4, 2023: contributorreACK 70303643cf42d18acbf1c020480c6bb23072dbd9siv2r commented at 4:55 pm on September 4, 2023: contributorreACK 7030364 (tests pass locally)real-or-random merged this on Sep 4, 2023real-or-random closed this on Sep 4, 2023
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 06:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me