tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID

[jonasnick]

Fixes #1167

[real-or-random]

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]

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]

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]

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.

[real-or-random]

(if you change it, also change it here)

[jonasnick]

• 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]

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]

The set_illegal_callback calls in ec_pubkey_parse_pointtest and run_eckey_edge_case_test should also be removed.

[real-or-random]

utACK fb5eb75

[siv2r]

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]

ACK fb5eb75, the tests locally pass on my machine (for all four commits).

[real-or-random]

needs rebase :)

[jonasnick]

rebased

[jonasnick]

@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]

reACK 7030364

[siv2r]

reACK 7030364 (tests pass locally)